[RFC PATCH] thinkpad_acpi: Add support for controlling charge thresholds

[Julian Andres Klode]

Add support for battery charge thresholds in new Sandy Bridge and Ivy Bridge
ThinkPads. Based on the unofficial documentation in tpacpi-bat.

The threshold files support the values '0' for the controller's default,
and 1-99 as percentages. Values outside of that range are rejected. The
behaviour of '0' might be confusing, especially for the stop case where
it basically seems to mean '100'.

Signed-off-by: Julian Andres Klode <j...@jak-linux.org>
---
Does this look OK so far? There are some other things I want to add
(force_discharge and others), but I thought I'd gather some comments
in case I'm doing it all wrong...

I'm slightly abusing struct dev_ext_attribute here, as I'm storing the
battery number in the 'var' pointer.

Note that I export the batteries starting at BAT0, but the EC starts
counting at 1 (0 is reserved for setting all batteries at once, but
that functionality is not provided here).

drivers/platform/x86/thinkpad_acpi.c | 158 +++++++++++++++++++++++++++++++++++
1 file changed, 158 insertions(+)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 03ca6c1..1e17222 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -528,6 +528,12 @@ static acpi_handle ec_handle;
TPACPI_HANDLE(ecrd, ec, "ECRD"); /* 570 */
TPACPI_HANDLE(ecwr, ec, "ECWR"); /* 570 */

+TPACPI_HANDLE(battery, root, "\\_SB.PCI0.LPC.EC.HKEY",
+ "\\_SB.PCI0.LPCB.EC.HKEY", /* X121e, T430u */
+ "\\_SB.PCI0.LPCB.H_EC.HKEY", /* L430 */
+ "\\_SB.PCI0.LPCB.EC0.HKEY", /* Edge/S series */
+ );
+
TPACPI_HANDLE(cmos, root, "\\UCMS", /* R50, R50e, R50p, R51, */
/* T4x, X31, X40 */
"\\CMOS", /* A3x, G4x, R32, T23, T30, X22-24, X30 */
@@ -8350,6 +8356,154 @@ static struct ibm_struct fan_driver_data = {
.resume = fan_resume,
};

+
+/*************************************************************************
+ * Battery subdriver
+ */
+
+/* Define a new battery, _BAT is a number >= 0 */
+#define DEFINE_BATTERY(_BAT) \
+static struct dev_ext_attribute bat##_BAT##_attribute_start_charge_thresh = { \
+ .attr = __ATTR(start_charge_tresh, (S_IWUSR | S_IRUGO), \
+ battery_start_charge_thresh_show, \
+ battery_start_charge_thresh_store), \
+ .var = (void *) (_BAT + 1) \
+}; \
+static struct dev_ext_attribute bat##_BAT##_attribute_stop_charge_thresh = { \
+ .attr = __ATTR(stop_charge_tresh, (S_IWUSR | S_IRUGO), \
+ battery_stop_charge_thresh_show, \
+ battery_stop_charge_thresh_store), \
+ .var = (void *) (_BAT + 1) \
+}; \
+static struct attribute *bat##_BAT##_attributes[] = { \
+ &bat##_BAT##_attribute_start_charge_thresh.attr.attr, \
+ &bat##_BAT##_attribute_stop_charge_thresh.attr.attr, \
+ NULL \
+}; \
+\
+static struct attribute_group bat##_BAT##_attribute_group = { \
+ .name = "BAT" #_BAT, \
+ .attrs = bat##_BAT##_attributes \
+};
+
+static int battery_attribute_get_battery(struct device_attribute *attr)
+{
+ return (int) (unsigned long) container_of(attr,
+ struct dev_ext_attribute,
+ attr)->var;
+}
+
+static ssize_t battery_start_charge_thresh_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int bat = battery_attribute_get_battery(attr);
+ int res = -EINVAL;
+ unsigned long value;
+
+ res = kstrtoul(buf, 0, &value);
+ if (res || value > 99)
+ return res ? res : -EINVAL;
+
+ if (!battery_handle || !acpi_evalf(battery_handle, &res, "BCCS",
+ "dd", (int) value | (bat << 8)))
+ return -EIO;
+
+ return count;
+}
+
+static ssize_t battery_stop_charge_thresh_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int bat = battery_attribute_get_battery(attr);
+ int res = -EINVAL;
+ unsigned long value;
+
+ res = kstrtoul(buf, 0, &value);
+ if (res || value > 99)
+ return res ? res : -EINVAL;
+
+ if (!battery_handle || !acpi_evalf(battery_handle, &res, "BCSS",
+ "dd", (int) value | (bat << 8)))
+ return -EIO;
+
+ return count;
+}
+
+static ssize_t battery_start_charge_thresh_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ int bat = battery_attribute_get_battery(attr);
+ int value;
+
+ if (!battery_handle || !acpi_evalf(battery_handle, &value, "BCTG",
+ "dd", bat))
+ return -EIO;
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", value & 0xFF);
+}
+
+static ssize_t battery_stop_charge_thresh_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ int bat = battery_attribute_get_battery(attr);
+ int value;
+
+ if (!battery_handle || !acpi_evalf(battery_handle, &value, "BCSG",
+ "dd", bat))
+ return -EIO;
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", value & 0xFF);
+}
+
+DEFINE_BATTERY(0);
+DEFINE_BATTERY(1);
+
+static struct attribute_group *bat_attribute_groups[] = {
+ &bat0_attribute_group,
+ &bat1_attribute_group,
+};
+
+static int __init battery_init(struct ibm_init_struct *iibm)
+{
+ int res;
+ int i;
+
+ vdbg_printk(TPACPI_DBG_INIT,
+ "initializing battery commands subdriver\n");
+
+ TPACPI_ACPIHANDLE_INIT(battery);
+
+ vdbg_printk(TPACPI_DBG_INIT, "battery commands are %s\n",
+ str_supported(battery_handle != NULL));
+
+ for (i = 0; i < ARRAY_SIZE(bat_attribute_groups); i++) {
+ res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
+ bat_attribute_groups[i]);
+ if (res)
+ return res;
+ }
+
+ return (battery_handle) ? 0 : 1;
+}
+
+static void battery_exit(void)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(bat_attribute_groups); i++)
+ sysfs_remove_group(&tpacpi_pdev->dev.kobj,
+ bat_attribute_groups[i]);
+}
+
+static struct ibm_struct battery_driver_data = {
+ .name = "battery",
+ .exit = battery_exit,
+};
+
/****************************************************************************
****************************************************************************
*
@@ -8741,6 +8895,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
.data = &light_driver_data,
},
{
+ .init = battery_init,
+ .data = &battery_driver_data,
+ },
+ {
.init = cmos_init,
.data = &cmos_driver_data,
},
--
1.8.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Bjørn Mork]

Julian Andres Klode <j...@jak-linux.org> writes:

>
> +TPACPI_HANDLE(battery, root, "\\_SB.PCI0.LPC.EC.HKEY",
> + "\\_SB.PCI0.LPCB.EC.HKEY", /* X121e, T430u */
> + "\\_SB.PCI0.LPCB.H_EC.HKEY", /* L430 */
> + "\\_SB.PCI0.LPCB.EC0.HKEY", /* Edge/S series */
> + );
> +

Isn't this just the full patch to the existing "hkey_handle" for those
models? Why not just use that handle, like e.g the rfkill driver does?

Supported models could probably be autodetected by checking whether the
methods exist?

> +static struct attribute_group bat##_BAT##_attribute_group = { \
> + .name = "BAT" #_BAT, \
> + .attrs = bat##_BAT##_attributes \
> +};

Are these names guaranteed to match the ACPI battery device(s)?

> +DEFINE_BATTERY(0);
> +DEFINE_BATTERY(1);

Are there always two batteries?



Bjørn

[Julian Andres Klode]

On Tue, Nov 05, 2013 at 11:18:02AM +0100, Bjørn Mork wrote:
> Julian Andres Klode <j...@jak-linux.org> writes:
>
> >
> > +TPACPI_HANDLE(battery, root, "\\_SB.PCI0.LPC.EC.HKEY",
> > + "\\_SB.PCI0.LPCB.EC.HKEY", /* X121e, T430u */
> > + "\\_SB.PCI0.LPCB.H_EC.HKEY", /* L430 */
> > + "\\_SB.PCI0.LPCB.EC0.HKEY", /* Edge/S series */
> > + );
> > +
>
> Isn't this just the full patch to the existing "hkey_handle" for those
> models? Why not just use that handle, like e.g the rfkill driver does?

I did not notice it, thanks for pointing that out.

>
> Supported models could probably be autodetected by checking whether the
> methods exist?

Yes, this makes more sense. I modified it locally to check for existence
of BCTG (get start threshold) and set tp_features.battery accordingly. If
it exists, all features are enabled (I think we can safely assume their
existence, I don't know if there are really thinkpads where you can get
a start threshold but don't have one of the other functions).

>
> > +static struct attribute_group bat##_BAT##_attribute_group = { \
> > + .name = "BAT" #_BAT, \
> > + .attrs = bat##_BAT##_attributes \
> > +};
>
> Are these names guaranteed to match the ACPI battery device(s)?

At least on the Sandy Bridge series and older, the first battery
(BAT0 here) always refers to the internal battery, and BAT1 to
the external one. I think this should match the ACPI battery
devices.

On the Haswell ones, I don't know, because they have one non-removable
built-in and one removable.

>
> > +DEFINE_BATTERY(0);
> > +DEFINE_BATTERY(1);
>
> Are there always two batteries?

As far as I can tell, the controller supports up to 2 batteries. And
they can be configured while they are not plugged in. So, exporting
both of them (all the time) makes sense.

I don't know if the W520 or W530 support 3 batteries, as I don't
have access to them. If they do, I don't know whether they will be
two separate entries or controlled by the same one.

--
Julian Andres Klode - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Please do not top-post if possible.

[Julian Andres Klode]

This patch series adds support for specifying charging thresholds,
forcing a battery to discharge, and inhibiting charging, on ThinkPad
Laptops using Sandy Bridge or newer processors.

The first two patches should be stable now, the third one does not work on
my test system (but I'd appreciate if anyone with something other than an
X230 tests it).

The inhibit charging part is a bit unprecise, and I can obviously not test
that -1 really means permanently, but I assume it does. I don't know whether
this one should be merged.

Changes since the RFC PATCH:
- Define batteries more dynamically instead of statically using large
macros. Added support for up to 3 batteries, unsupported ones will not
be exported.
- Added 3 more new patches with more features

Julian Andres Klode (4):
thinkpad_acpi: Add support for controlling charge thresholds
thinkpad_acpi: battery: Add force_discharge attribute
thinkpad_acpi: battery: Add force_discharge_ac_break attribute
thinkpad_acpi: battery: Add inhibit_charge_minutes attribute

Documentation/laptops/thinkpad-acpi.txt | 37 ++++
drivers/platform/x86/thinkpad_acpi.c | 300 ++++++++++++++++++++++++++++++++
2 files changed, 337 insertions(+)

--
1.8.4.2

[Julian Andres Klode]

This makes it possible to forcefully discharge a battery.

Signed-off-by: Julian Andres Klode <j...@jak-linux.org>
---

Documentation/laptops/thinkpad-acpi.txt | 4 +++
drivers/platform/x86/thinkpad_acpi.c | 49 ++++++++++++++++++++++++++++++++-
2 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/Documentation/laptops/thinkpad-acpi.txt b/Documentation/laptops/thinkpad-acpi.txt
index 97b150a..72882bb 100644
--- a/Documentation/laptops/thinkpad-acpi.txt
+++ b/Documentation/laptops/thinkpad-acpi.txt
@@ -1370,6 +1370,10 @@ battery sysfs attribute: stop_charge_thresh
A percentage value from 1-99%, controlling when charging should stop, or
0 for the default value.

+battery sysfs attribute: force_discharge
+
+ Force discharging (0 or 1)
+
Multiple Commands, Module Parameters
------------------------------------

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 66e629e..67ce469 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8358,7 +8358,7 @@ static struct ibm_struct fan_driver_data = {

/* Modify battery_init() if you modify them */
#define BATTERY_MAX_COUNT 3
-#define BATTERY_MAX_ATTRS 2
+#define BATTERY_MAX_ATTRS 3

static struct battery {
char name[3 + 1 + 1];
@@ -8437,6 +8437,46 @@ static ssize_t battery_stop_charge_thresh_show(struct device *dev,

return snprintf(buf, PAGE_SIZE, "%d\n", value & 0xFF);
}

+static ssize_t battery_force_discharge_show(struct device *dev,

+ struct device_attribute *attr,
+ char *buf)
+{
+ int bat = battery_attribute_get_battery(attr);
+ int value;
+

+ if (!hkey_handle || !acpi_evalf(hkey_handle, &value, "BDSG",

+ "dd", bat))
+ return -EIO;
+

+ return snprintf(buf, PAGE_SIZE, "%d\n", value & 0x1);
+}
+
+static ssize_t battery_force_discharge_store(struct device *dev,

+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int bat = battery_attribute_get_battery(attr);
+ int res = -EINVAL;
+ unsigned long value;
+
+ res = kstrtoul(buf, 0, &value);

+ if (res || value > 1)

+ return res ? res : -EINVAL;
+

+ if (!hkey_handle || !acpi_evalf(hkey_handle, &res, "BDSG", "dd", bat))
+ return -EIO;
+
+ /* Keep the break on AC disconnect state */
+ value |= (res >> 1) & 0x1;
+ /* Set the battery */
+ value |= (bat << 8);
+
+ if (!hkey_handle || !acpi_evalf(hkey_handle, &res, "BDSS",
+ "dd", (int) value) || res < 0)
+ return -EIO;

+ return count;
+}
+

static int __init battery_init(struct ibm_init_struct *iibm)

{
int res;
@@ -8482,6 +8522,13 @@ static int __init battery_init(struct ibm_init_struct *iibm)
battery_stop_charge_thresh_store),
.var = (void *) (unsigned long) (i + 1)
};
+ batteries[i].attributes[j++] = (struct dev_ext_attribute) {
+ .attr = __ATTR(force_discharge,
+ S_IWUSR | S_IRUGO,
+ battery_force_discharge_show,
+ battery_force_discharge_store),
+ .var = (void *) (unsigned long) (i + 1)
+ };

strncpy(batteries[i].name, "BAT", 3);
batteries[i].name[3] = '0' + i;

[Julian Andres Klode]

Add support for battery charge thresholds in new Sandy Bridge and Ivy Bridge
ThinkPads. Based on the unofficial documentation in tpacpi-bat.

The threshold files support the values '0' for the controller's default,
and 1-99 as percentages. Values outside of that range are rejected. The
behaviour of '0' might be confusing, especially for the stop case where
it basically seems to mean '100'.

Signed-off-by: Julian Andres Klode <j...@jak-linux.org>
---

Documentation/laptops/thinkpad-acpi.txt | 17 ++++
drivers/platform/x86/thinkpad_acpi.c | 173 ++++++++++++++++++++++++++++++++
2 files changed, 190 insertions(+)

diff --git a/Documentation/laptops/thinkpad-acpi.txt b/Documentation/laptops/thinkpad-acpi.txt
index 86c5236..97b150a 100644
--- a/Documentation/laptops/thinkpad-acpi.txt
+++ b/Documentation/laptops/thinkpad-acpi.txt
@@ -46,6 +46,7 @@ detailed description):
- Fan control and monitoring: fan speed, fan enable/disable
- WAN enable and disable
- UWB enable and disable
+ - Charging control

A compatibility table by model and feature is maintained on the web
site, http://ibm-acpi.sf.net/. I appreciate any success or failure
@@ -1352,6 +1353,22 @@ Sysfs notes:
rfkill controller switch "tpacpi_uwb_sw": refer to
Documentation/rfkill.txt for details.

+Charging control
+----------------
+sysfs attribute groups: BAT0, BAT1, and so on, depending on how many batteries
+are supported by the embedded controller.
+
+This feature controls the battery charging process.
+
+battery sysfs attribute: start_charge_thresh
+
+ A percentage value from 1-99%, controlling when charging should start, or
+ 0 for the default value.
+
+battery sysfs attribute: stop_charge_thresh
+
+ A percentage value from 1-99%, controlling when charging should stop, or
+ 0 for the default value.

Multiple Commands, Module Parameters
------------------------------------
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c

index 03ca6c1..66e629e 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -323,6 +323,7 @@ static struct {
u32 sensors_pdrv_attrs_registered:1;
u32 sensors_pdev_attrs_registered:1;
u32 hotkey_poll_active:1;
+ u32 battery:1;
} tp_features;

static struct {
@@ -8350,6 +8351,174 @@ static struct ibm_struct fan_driver_data = {

.resume = fan_resume,
};

+
+/*************************************************************************
+ * Battery subdriver
+ */
+

+/* Modify battery_init() if you modify them */
+#define BATTERY_MAX_COUNT 3
+#define BATTERY_MAX_ATTRS 2
+
+static struct battery {
+ char name[3 + 1 + 1];
+ struct attribute_set *set;
+ struct dev_ext_attribute attributes[BATTERY_MAX_ATTRS];
+} batteries[BATTERY_MAX_COUNT];

+
+static int battery_attribute_get_battery(struct device_attribute *attr)
+{
+ return (int) (unsigned long) container_of(attr,
+ struct dev_ext_attribute,
+ attr)->var;
+}
+

+static ssize_t battery_start_charge_thresh_store(struct device *dev,

+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int bat = battery_attribute_get_battery(attr);
+ int res = -EINVAL;
+ unsigned long value;
+
+ res = kstrtoul(buf, 0, &value);

+ if (res || value > 99)

+ return res ? res : -EINVAL;
+

+ if (!hkey_handle || !acpi_evalf(hkey_handle, &res, "BCCS", "dd",
+ (int) value | (bat << 8)) || res < 0)

+ return -EIO;
+ return count;
+}
+

+static ssize_t battery_stop_charge_thresh_store(struct device *dev,

+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int bat = battery_attribute_get_battery(attr);
+ int res = -EINVAL;
+ unsigned long value;
+
+ res = kstrtoul(buf, 0, &value);

+ if (res || value > 99)

+ return res ? res : -EINVAL;
+

+ if (!hkey_handle || !acpi_evalf(hkey_handle, &res, "BCSS", "dd",
+ (int) value | (bat << 8)) || res < 0)

+ return -EIO;
+ return count;
+}
+

+static ssize_t battery_start_charge_thresh_show(struct device *dev,

+ struct device_attribute *attr,
+ char *buf)
+{
+ int bat = battery_attribute_get_battery(attr);
+ int value;
+

+ if (!hkey_handle || !acpi_evalf(hkey_handle, &value, "BCTG",

+ "dd", bat))
+ return -EIO;
+

+ return snprintf(buf, PAGE_SIZE, "%d\n", value & 0xFF);
+}
+

+static ssize_t battery_stop_charge_thresh_show(struct device *dev,

+ struct device_attribute *attr,
+ char *buf)
+{
+ int bat = battery_attribute_get_battery(attr);
+ int value;
+

+ if (!hkey_handle || !acpi_evalf(hkey_handle, &value, "BCSG",

+ "dd", bat))
+ return -EIO;
+

+ return snprintf(buf, PAGE_SIZE, "%d\n", value & 0xFF);
+}

+
+static int __init battery_init(struct ibm_init_struct *iibm)
+{
+ int res;
+ int i;

+ int state;

+
+ vdbg_printk(TPACPI_DBG_INIT,
+ "initializing battery commands subdriver\n");
+

+ TPACPI_ACPIHANDLE_INIT(hkey);
+
+ /* Check whether getter for start threshold exists */
+ tp_features.battery = hkey_handle &&
+ acpi_evalf(hkey_handle, &state, "BCTG", "qdd", 1);

+
+ vdbg_printk(TPACPI_DBG_INIT, "battery commands are %s\n",

+ str_supported(tp_features.battery));
+
+ if (!tp_features.battery)
+ return 1;
+
+ for (i = 0; i < BATTERY_MAX_COUNT; i++) {
+ int j = 0;
+ if (!acpi_evalf(hkey_handle, &state, "BCTG", "qdd", i + 1))
+ continue;
+ /* If the sign bit was set, we could not get the start charge
+ * threshold of that battery. Let's assume that this battery
+ * (and all following ones) do not exist */
+ if (state < 0)
+ break;
+ /* Modify BATTERY_MAX_ATTRS if you add an attribute */

+ batteries[i].attributes[j++] = (struct dev_ext_attribute) {

+ .attr = __ATTR(start_charge_tresh,
+ S_IWUSR | S_IRUGO,
+ battery_start_charge_thresh_show,
+ battery_start_charge_thresh_store),

+ .var = (void *) (unsigned long) (i + 1)
+ };

+ batteries[i].attributes[j++] = (struct dev_ext_attribute) {

+ .attr = __ATTR(stop_charge_tresh,
+ S_IWUSR | S_IRUGO,
+ battery_stop_charge_thresh_show,
+ battery_stop_charge_thresh_store),

+ .var = (void *) (unsigned long) (i + 1)
+ };

+

+ strncpy(batteries[i].name, "BAT", 3);

+ batteries[i].name[3] = '0' + i;
+ batteries[i].name[4] = '\0';
+ batteries[i].set = create_attr_set(j - 1, batteries[i].name);
+
+ for (j = j - 1; j >= 0; j--)
+ add_to_attr_set(batteries[i].set,
+ &batteries[i].attributes[j].attr.attr);
+
+ res = register_attr_set_with_sysfs(batteries[i].set,
+ &tpacpi_pdev->dev.kobj);
+

+ if (res)
+ return res;
+ }
+

+ return 0;

+}
+
+static void battery_exit(void)
+{
+ int i;
+

+ for (i = 0; i < BATTERY_MAX_COUNT; i++) {
+ if (batteries[i].set != NULL) {
+ delete_attr_set(batteries[i].set,
+ &tpacpi_pdev->dev.kobj);
+ }
+ }

+}
+
+static struct ibm_struct battery_driver_data = {
+ .name = "battery",
+ .exit = battery_exit,
+};
+
/****************************************************************************
****************************************************************************
*

@@ -8741,6 +8910,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {

.data = &light_driver_data,
},
{
+ .init = battery_init,
+ .data = &battery_driver_data,
+ },
+ {
.init = cmos_init,
.data = &cmos_driver_data,
},

[Julian Andres Klode]

This should be considered experimental. There seem to be to ways to
inhibit charging, one globally using peak shift state, and one using
a battery-specific inhibit charge command. The latter is implemented
here. Setting a permanent timeout is not supported currently.

Reading the timeout is not supported because the ACPI call needed
to read the value is not documented/reverse-engineered yet.

Signed-off-by: Julian Andres Klode <j...@jak-linux.org>
---

Documentation/laptops/thinkpad-acpi.txt | 12 ++++++++++++
drivers/platform/x86/thinkpad_acpi.c | 34 ++++++++++++++++++++++++++++++++-
2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/Documentation/laptops/thinkpad-acpi.txt b/Documentation/laptops/thinkpad-acpi.txt
index 8ec1898..e639f72 100644
--- a/Documentation/laptops/thinkpad-acpi.txt
+++ b/Documentation/laptops/thinkpad-acpi.txt
@@ -1378,6 +1378,18 @@ battery sysfs attribute: force_discharge_ac_break

Break the discharging when the AC disconnects (0 or 1)

+battery sysfs attribute: inhibit_charge_minutes
+
+ Inhibit charging for the specified amount of minutes. This attribute
+ may not be very precise, the charging may start again some seconds
+ earlier than requested. It's also write-only until someone figured out
+ how reading that value works.
+
+ Values range: -1 to 720
+
+ The value -1 is supposed to mean permanently.
+
+

Multiple Commands, Module Parameters
------------------------------------

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c

index 6b0521a..a9cba4e 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8358,7 +8358,7 @@ static struct ibm_struct fan_driver_data = {

/* Modify battery_init() if you modify them */

#define BATTERY_MAX_COUNT 3
-#define BATTERY_MAX_ATTRS 4
+#define BATTERY_MAX_ATTRS 5

static struct battery {

char name[3 + 1 + 1];

@@ -8518,6 +8518,31 @@ static ssize_t battery_force_discharge_ac_break_store(struct device *dev,
return count;
}

+static ssize_t battery_inhibit_charge_minutes_store(struct device *dev,

+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int bat = battery_attribute_get_battery(attr);
+ int res = -EINVAL;

+ int value;
+
+ res = kstrtoint(buf, 0, &value);
+ if (res || value > 720 || value < -1)

+ return res ? res : -EINVAL;
+

+ if (value == -1)
+ value = 1;
+ else
+ value = (value << 8) | (value & 1);
+
+ value |= (bat << 4);
+
+ if (!hkey_handle || !acpi_evalf(hkey_handle, &res, "BICS",
+ "dd", (int) value) || res < 0)

+ return -EIO;
+ return count;
+}
+

static int __init battery_init(struct ibm_init_struct *iibm)

{
int res;
@@ -8577,6 +8602,13 @@ static int __init battery_init(struct ibm_init_struct *iibm)
battery_force_discharge_ac_break_store),

.var = (void *) (unsigned long) (i + 1)
};

+ batteries[i].attributes[j++] = (struct dev_ext_attribute) {

+ .attr = __ATTR(inhibit_charge_minutes,
+ S_IWUSR,
+ NULL,
+ battery_inhibit_charge_minutes_store),

+ .var = (void *) (unsigned long) (i + 1)
+ };

strncpy(batteries[i].name, "BAT", 3);

batteries[i].name[3] = '0' + i;

[Julian Andres Klode]

Tells the EC to stop a forceful discharging if the AC is disconnected. First
needs force_discharge set to 1 to be able to work. Actual functionality is
untested, as that attribute is not supported on an X230.

Signed-off-by: Julian Andres Klode <j...@jak-linux.org>
---

Documentation/laptops/thinkpad-acpi.txt | 4 +++
drivers/platform/x86/thinkpad_acpi.c | 50 ++++++++++++++++++++++++++++++++-
2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/Documentation/laptops/thinkpad-acpi.txt b/Documentation/laptops/thinkpad-acpi.txt
index 72882bb..8ec1898 100644
--- a/Documentation/laptops/thinkpad-acpi.txt
+++ b/Documentation/laptops/thinkpad-acpi.txt
@@ -1374,6 +1374,10 @@ battery sysfs attribute: force_discharge

Force discharging (0 or 1)

+battery sysfs attribute: force_discharge_ac_break
+
+ Break the discharging when the AC disconnects (0 or 1)

+
Multiple Commands, Module Parameters
------------------------------------

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c

index 67ce469..6b0521a 100644

--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8358,7 +8358,7 @@ static struct ibm_struct fan_driver_data = {

/* Modify battery_init() if you modify them */
#define BATTERY_MAX_COUNT 3

-#define BATTERY_MAX_ATTRS 3
+#define BATTERY_MAX_ATTRS 4

static struct battery {
char name[3 + 1 + 1];

@@ -8477,6 +8477,47 @@ static ssize_t battery_force_discharge_store(struct device *dev,
return count;
}

+static ssize_t battery_force_discharge_ac_break_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)

+{
+ int bat = battery_attribute_get_battery(attr);

+ int value;
+
+ if (!hkey_handle || !acpi_evalf(hkey_handle, &value, "BDSG",

+ "dd", bat))
+ return -EIO;
+

+ return snprintf(buf, PAGE_SIZE, "%d\n", (value >> 1) & 0x1);
+}
+
+static ssize_t battery_force_discharge_ac_break_store(struct device *dev,

+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int bat = battery_attribute_get_battery(attr);
+ int res = -EINVAL;

+ unsigned long value;
+
+ res = kstrtoul(buf, 0, &value);
+ if (res || value > 1)

+ return res ? res : -EINVAL;
+

+ if (!hkey_handle || !acpi_evalf(hkey_handle, &res, "BDSG", "dd", bat))
+ return -EIO;
+
+ /* Move into the right position */
+ value <<= 1;
+ /* Keep the forcedness state */
+ value |= (res) & 0x1;

+ /* Set the battery */
+ value |= (bat << 8);

+ if (!hkey_handle || !acpi_evalf(hkey_handle, &res, "BDSS",

+ "dd", (int) value) || res < 0)
+ return -EIO;
+ return count;
+}
+
static int __init battery_init(struct ibm_init_struct *iibm)
{
int res;

@@ -8529,6 +8570,13 @@ static int __init battery_init(struct ibm_init_struct *iibm)
battery_force_discharge_store),

.var = (void *) (unsigned long) (i + 1)
};
+ batteries[i].attributes[j++] = (struct dev_ext_attribute) {

+ .attr = __ATTR(force_discharge_ac_break,
+ S_IWUSR | S_IRUGO,
+ battery_force_discharge_ac_break_show,
+ battery_force_discharge_ac_break_store),

+ .var = (void *) (unsigned long) (i + 1)
+ };

strncpy(batteries[i].name, "BAT", 3);
batteries[i].name[3] = '0' + i;

[Julian Andres Klode]

On Mon, Nov 11, 2013 at 02:56:30PM +0100, Julian Andres Klode wrote:
> +static int __init battery_init(struct ibm_init_struct *iibm)

There's a bug here that I just noticed: It should be (j) attributes, not (j - 1)
ones. The following patch fixes this. I can squash it into that patch and resubmit
later on if requested.

-- >8 --
Subject: [PATCH] thinkpad_acpi: battery: Fix the size of the battery attribute sets

There are not j - 1, but j attributes.

Signed-off-by: Julian Andres Klode <j...@jak-linux.org>
---

drivers/platform/x86/thinkpad_acpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index a9cba4e..6948141 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8613,7 +8613,7 @@ static int __init battery_init(struct ibm_init_struct *iibm)

strncpy(batteries[i].name, "BAT", 3);

batteries[i].name[3] = '0' + i;

batteries[i].name[4] = '\0';

- batteries[i].set = create_attr_set(j - 1, batteries[i].name);
+ batteries[i].set = create_attr_set(j, batteries[i].name);

for (j = j - 1; j >= 0; j--)

add_to_attr_set(batteries[i].set,
--
1.8.4.2

--
Julian Andres Klode - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Please do not top-post if possible.

[Julian Andres Klode]

On Mon, Nov 11, 2013 at 02:56:29PM +0100, Julian Andres Klode wrote:
> This patch series adds support for specifying charging thresholds,
> forcing a battery to discharge, and inhibiting charging, on ThinkPad
> Laptops using Sandy Bridge or newer processors.
>
> The first two patches should be stable now, the third one does not work on
> my test system (but I'd appreciate if anyone with something other than an
> X230 tests it).
>
> The inhibit charging part is a bit unprecise, and I can obviously not test
> that -1 really means permanently, but I assume it does. I don't know whether
> this one should be merged.
>
> Changes since the RFC PATCH:
> - Define batteries more dynamically instead of statically using large
> macros. Added support for up to 3 batteries, unsupported ones will not
> be exported.
> - Added 3 more new patches with more features
>
> Julian Andres Klode (4):
> thinkpad_acpi: Add support for controlling charge thresholds
> thinkpad_acpi: battery: Add force_discharge attribute
> thinkpad_acpi: battery: Add force_discharge_ac_break attribute
> thinkpad_acpi: battery: Add inhibit_charge_minutes attribute
>
> Documentation/laptops/thinkpad-acpi.txt | 37 ++++
> drivers/platform/x86/thinkpad_acpi.c | 300 ++++++++++++++++++++++++++++++++
> 2 files changed, 337 insertions(+)
>

I'd really like to get some comments on this series (especially by Henrique
and Matthew, they're responsible); so I know what I have to fix and whether
I should drop some of the later patches, and if the patch series has any
chance of being accepted...

--
Julian Andres Klode - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Please do not top-post if possible.

[Julian Andres Klode]

On Mon, Nov 11, 2013 at 02:56:29PM +0100, Julian Andres Klode wrote:

> This patch series adds support for specifying charging thresholds,
> forcing a battery to discharge, and inhibiting charging, on ThinkPad
> Laptops using Sandy Bridge or newer processors.

Hello again,

it's been about 1.5 months now after I submitted this patch series
and I have not received any response yet.

I fixed the issues pointed out by Bj�rn Mork in the previous patch
except for the one with the battery names, as I have no idea how
battery numbers are created or mapped. I can only say that the names
are the same on my test machine, I don't have any other ones.

Henrique, it would be really great if you could take a look.

--
Julian Andres Klode - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Please do not top-post if possible.

[Henrique de Moraes Holschuh]

On Sat, 28 Dec 2013, Julian Andres Klode wrote:
> On Mon, Nov 11, 2013 at 02:56:29PM +0100, Julian Andres Klode wrote:
> > This patch series adds support for specifying charging thresholds,
> > forcing a battery to discharge, and inhibiting charging, on ThinkPad
> > Laptops using Sandy Bridge or newer processors.
>
> Hello again,
>
> it's been about 1.5 months now after I submitted this patch series
> and I have not received any response yet.
>
> I fixed the issues pointed out by Bj�rn Mork in the previous patch
> except for the one with the battery names, as I have no idea how
> battery numbers are created or mapped. I can only say that the names
> are the same on my test machine, I don't have any other ones.
>
> Henrique, it would be really great if you could take a look.

I did. I just did not manage to finish looking over it, it is not really
okay as-is, mostly because it did not create a power-supply device and
properly extended that thing in a generic way, which would be vastly
preferable to adding yet another private interface.

--
"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

[Julian Andres Klode]

On Sat, Dec 28, 2013 at 08:10:03PM -0200, Henrique de Moraes Holschuh wrote:
> On Sat, 28 Dec 2013, Julian Andres Klode wrote:
> > On Mon, Nov 11, 2013 at 02:56:29PM +0100, Julian Andres Klode wrote:
> > > This patch series adds support for specifying charging thresholds,
> > > forcing a battery to discharge, and inhibiting charging, on ThinkPad
> > > Laptops using Sandy Bridge or newer processors.
> >

[...]

> > Henrique, it would be really great if you could take a look.
>
> I did. I just did not manage to finish looking over it, it is not really
> okay as-is, mostly because it did not create a power-supply device and
> properly extended that thing in a generic way, which would be vastly
> preferable to adding yet another private interface.

Thanks for the response.

I think that a more generic approach is a good idea, but
I don't think creating a new power supply device would be the right
choice because this controls existing supplies rather than being a
supply in itself, and I am not aware of any other kind of power
supply device that allows setting things.

We could extend the existing battery devices and then get paths like
/sys/class/power_supply/BAT0/start_charge_thresh
which looks `natural'. There is one problem with that approach, though:

A ThinkPad supports multiple batteries: In my case, I could add a slice
battery. But the ACPI driver only exposes a BAT0 device without a slice
being attached (I assume a BAT1 will pop-up if you add a slice battery,
but can't test it, as I don't own one). This means that you can only
configure the slice battery when it's plugged in. I don't know if its
a good idea to work like this.

Another problem is that I don't know any other systems supporting charge
thresholds, and I don't want to dictate interfaces built for ThinkPads
as a generic way.

--
Julian Andres Klode - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Please do not top-post if possible.

[Julian Andres Klode]

On Mon, Nov 11, 2013 at 02:56:30PM +0100, Julian Andres Klode wrote:
> Add support for battery charge thresholds in new Sandy Bridge and Ivy Bridge
> ThinkPads. Based on the unofficial documentation in tpacpi-bat.
>
> The threshold files support the values '0' for the controller's default,
> and 1-99 as percentages. Values outside of that range are rejected. The
> behaviour of '0' might be confusing, especially for the stop case where
> it basically seems to mean '100'.

Thinking more about this, it might make more sense to simply accept a 100
value and not accept a 0 value in the stop case (I tried multiple times to
write 100 to a stop_charge_thresh file, because that feels more natural).

Having a 0 mean 100% is just odd. So stop_charge_thresh should simply
accepts integer values in the range [1, 100] (and start_charge_thresh
should continue accept [0, 99], as 0 really means 0 there).

--
Julian Andres Klode - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Please do not top-post if possible.

[Henrique de Moraes Holschuh]

On Mon, 30 Dec 2013, Julian Andres Klode wrote:
> I think that a more generic approach is a good idea, but
> I don't think creating a new power supply device would be the right

...

> We could extend the existing battery devices and then get paths like
> /sys/class/power_supply/BAT0/start_charge_thresh
> which looks `natural'. There is one problem with that approach, though:

I'm fine with extending battery devices, in fact I agree that it would be a
better place to add these attributes to.

> A ThinkPad supports multiple batteries: In my case, I could add a slice
> battery. But the ACPI driver only exposes a BAT0 device without a slice
> being attached (I assume a BAT1 will pop-up if you add a slice battery,

Thinkpad batteries are static, BAT1 is always BAT1 for the firmware,
regardless of whether BAT0 is present or not. Their parameters can always
be set as well, since they're stored in the EC and not in the battery pack
uC. And the EC doesn't care whether the battery is present or not.

> but can't test it, as I don't own one). This means that you can only
> configure the slice battery when it's plugged in. I don't know if its
> a good idea to work like this.

We could change the ACPI battery driver to optionally allow a platform
driver (or a dmi table) to order it to register batteries that are not
present at boot, and set that flag for thinkpads.

> Another problem is that I don't know any other systems supporting charge
> thresholds, and I don't want to dictate interfaces built for ThinkPads
> as a generic way.

All SBS batteries support it, which means most notebooks do. Whether they
expose it or not, and whether their platform drivers know about it or not in
order to expose these parameters to the kernel and to userspace, is a
separate issue.

Refer to: http://www.sbs-forum.org/

So yes, we can and should add these as generic *optional* parameters for
generic batteries. How to best add it to the ACPI battery driver is
something to think about.

--
"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

[Henrique de Moraes Holschuh]

On Mon, 30 Dec 2013, Julian Andres Klode wrote:

> On Mon, Nov 11, 2013 at 02:56:30PM +0100, Julian Andres Klode wrote:
> > Add support for battery charge thresholds in new Sandy Bridge and Ivy Bridge
> > ThinkPads. Based on the unofficial documentation in tpacpi-bat.
> >
> > The threshold files support the values '0' for the controller's default,
> > and 1-99 as percentages. Values outside of that range are rejected. The
> > behaviour of '0' might be confusing, especially for the stop case where
> > it basically seems to mean '100'.
>
> Thinking more about this, it might make more sense to simply accept a 100
> value and not accept a 0 value in the stop case (I tried multiple times to
> write 100 to a stop_charge_thresh file, because that feels more natural).
>
> Having a 0 mean 100% is just odd. So stop_charge_thresh should simply
> accepts integer values in the range [1, 100] (and start_charge_thresh
> should continue accept [0, 99], as 0 really means 0 there).

Indeed. Just return EINVAL for a stop threshold of 0.

--
"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

[Henrique de Moraes Holschuh]

On Mon, 30 Dec 2013, Henrique de Moraes Holschuh wrote:
> On Mon, 30 Dec 2013, Julian Andres Klode wrote:
> > On Mon, Nov 11, 2013 at 02:56:30PM +0100, Julian Andres Klode wrote:
> > > Add support for battery charge thresholds in new Sandy Bridge and Ivy Bridge
> > > ThinkPads. Based on the unofficial documentation in tpacpi-bat.
> > >
> > > The threshold files support the values '0' for the controller's default,
> > > and 1-99 as percentages. Values outside of that range are rejected. The
> > > behaviour of '0' might be confusing, especially for the stop case where
> > > it basically seems to mean '100'.
> >
> > Thinking more about this, it might make more sense to simply accept a 100
> > value and not accept a 0 value in the stop case (I tried multiple times to
> > write 100 to a stop_charge_thresh file, because that feels more natural).
> >
> > Having a 0 mean 100% is just odd. So stop_charge_thresh should simply
> > accepts integer values in the range [1, 100] (and start_charge_thresh
> > should continue accept [0, 99], as 0 really means 0 there).
>
> Indeed. Just return EINVAL for a stop threshold of 0.

Actually, a stop threshold below the current start threshold is also invalid
and what happens when you try that is implementation-specific. And the
kernel is not the only party who can change the thresholds, so you have to
read them if you really need to know the current value.

To correct myself, stop threshold being zero is also implementation-
specific. on thinkpads it is not valid.

For thinkpads, I believe the EC firmware changes the other threshold so that
the boundary condition stop > start is always valid. But I never tried it
with a direct EC write to validate this. If it becomes important, I can
check -- but I'd still prefer to enforce sanity at the driver level just in
case. Don't tempt the gremilins, for they live at boundary conditions and
their sleep is light indeed.

[Julian Andres Klode]

On Mon, Dec 30, 2013 at 08:40:45PM -0200, Henrique de Moraes Holschuh wrote:
> On Mon, 30 Dec 2013, Henrique de Moraes Holschuh wrote:
> > On Mon, 30 Dec 2013, Julian Andres Klode wrote:
> > > On Mon, Nov 11, 2013 at 02:56:30PM +0100, Julian Andres Klode wrote:
> > > > Add support for battery charge thresholds in new Sandy Bridge and Ivy Bridge
> > > > ThinkPads. Based on the unofficial documentation in tpacpi-bat.
> > > >
> > > > The threshold files support the values '0' for the controller's default,
> > > > and 1-99 as percentages. Values outside of that range are rejected. The
> > > > behaviour of '0' might be confusing, especially for the stop case where
> > > > it basically seems to mean '100'.
> > >
> > > Thinking more about this, it might make more sense to simply accept a 100
> > > value and not accept a 0 value in the stop case (I tried multiple times to
> > > write 100 to a stop_charge_thresh file, because that feels more natural).
> > >
> > > Having a 0 mean 100% is just odd. So stop_charge_thresh should simply
> > > accepts integer values in the range [1, 100] (and start_charge_thresh
> > > should continue accept [0, 99], as 0 really means 0 there).
> >
> > Indeed. Just return EINVAL for a stop threshold of 0.
>
> Actually, a stop threshold below the current start threshold is also invalid
> and what happens when you try that is implementation-specific. And the
> kernel is not the only party who can change the thresholds, so you have to
> read them if you really need to know the current value.

That's true. The problem is that if we forbid such values, the writes
will need to be done in a specific order. Let's say I have a system
with
start=0 stop=50
and I want to change it to
start=60 stop=80

If we return -EINVAL on a case stop < start, the only way to do this
is setting stop first and then start, because if we wrote start first,
then we would have: start > stop (because 60 > 50). But the error
message when doing it the wrong way around will not be very helpful
and I think it is confusing.

We might be able to work around this by simple setting stop = start
if a new write causes the stop threshold to be below the start
threshold. But this also seems ugly.

> To correct myself, stop threshold being zero is also implementation-
> specific. on thinkpads it is not valid.

Writing the value 0 effectively means setting the stop threshold to 100%.
The EC itself does not accept the value 100. I don't know why they choose
to implement it this way. So the code should not accept 0 as an input
value and replace an input of 100 with 0.

The exact value returned by the ACPI call is 0x80000000, that is,
a signed 0 integer with sign bit set to 1. A set sign bit indicates
an error.

>
> For thinkpads, I believe the EC firmware changes the other threshold so that
> the boundary condition stop > start is always valid. But I never tried it
> with a direct EC write to validate this. If it becomes important, I can
> check -- but I'd still prefer to enforce sanity at the driver level just in
> case. Don't tempt the gremilins, for they live at boundary conditions and
> their sleep is light indeed.

It does not seem to do this here.

--
Julian Andres Klode - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Please do not top-post if possible.

[Henrique de Moraes Holschuh]

On Tue, 31 Dec 2013, Julian Andres Klode wrote:
> We might be able to work around this by simple setting stop = start
> if a new write causes the stop threshold to be below the start
> threshold. But this also seems ugly.

It is the safest way, but the correct pseudo-code would be, assuiming
unsigned:

when someone changes start:

if (start > 99)
start = 99;
get_current_stop_treshold(&stop);
if (start > stop)
stop = start + 1;
set_thresholds(start, stop);


when someone changes stop:

if (stop == 0 || stop > 100)
stop = 100;
get_current_start_threshold(&start);
if (start > stop)
start = stop - 1;
set_thresholds(start, stop);

And write the stop threshold mod 100 at the low-level "send thresholds to
the firmware" routine.

> Writing the value 0 effectively means setting the stop threshold to 100%.
> The EC itself does not accept the value 100. I don't know why they choose
> to implement it this way. So the code should not accept 0 as an input
> value and replace an input of 100 with 0.

Yes. This behaviour exists since start/stop thresholds were first exported
outside the EC, in the X30 or thereabouts.

> > For thinkpads, I believe the EC firmware changes the other threshold so that
> > the boundary condition stop > start is always valid. But I never tried it
> > with a direct EC write to validate this. If it becomes important, I can
> > check -- but I'd still prefer to enforce sanity at the driver level just in
> > case. Don't tempt the gremilins, for they live at boundary conditions and
> > their sleep is light indeed.
>
> It does not seem to do this here.

Then it is best if we enforce it in the driver. I've crashed thinkpad ECs
before, *don't tempt the gremilins* indeed.

--
"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

[Julian Andres Klode]

On Tue, Dec 31, 2013 at 10:12:31AM -0200, Henrique de Moraes Holschuh wrote:
> On Tue, 31 Dec 2013, Julian Andres Klode wrote:
> > We might be able to work around this by simple setting stop = start
> > if a new write causes the stop threshold to be below the start
> > threshold. But this also seems ugly.
>
> It is the safest way, but the correct pseudo-code would be, assuiming
> unsigned:
>
> when someone changes start:
>
> if (start > 99)
> start = 99;

I think we should just return -EINVAL in such cases. Allowing users to
write larger percentages is a bit pointless (we don't allow them to write
negative ones either). And other promiment code (the backlight drivers)
seem to reject out-of-range values.

> set_thresholds(start, stop);

I think there should not be some common set_thresholds, because we also
need to write things in different orders for start / stop then:

DECREASE STOP => Write new start if needed, then write stop
INCREASE START => Write new stop if needed, then write start

Otherwise we might have a very very very short time in which start
is greater than stop.

I'll incorporate this in real code and test it tomorrow.

Sometimes after that, I'd like to tackle the integration with power_supply.
This is a bit more complicated, there even appear to be two battery drivers
in ACPI, namely battery and sbs, and battery is used on my system (I don't
know if that's the generic case on ThinkPads, or if is the case because
battery is builtin and sbs is not). How to do extend that is probably
something best discussed with ACPI maintainers, as I can't see
any mechanism for this.

--
Julian Andres Klode - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Please do not top-post if possible.

[Pavel Machek]

Hi!

> Another problem is that I don't know any other systems supporting charge
> thresholds, and I don't want to dictate interfaces built for ThinkPads
> as a generic way.

On systems like Sharp Zaurus, Nokia N900, charging is controlled by
Linux. As charge thresholds should extend Li-ION battery life under
anticipated light usage, they are probably good idea on all systems
that can support them.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[Julian Andres Klode]

On Tue, Dec 31, 2013 at 11:46:05PM +0100, Julian Andres Klode wrote:
> On Tue, Dec 31, 2013 at 10:12:31AM -0200, Henrique de Moraes Holschuh wrote:
> > On Tue, 31 Dec 2013, Julian Andres Klode wrote:
> > > We might be able to work around this by simple setting stop = start
> > > if a new write causes the stop threshold to be below the start
> > > threshold. But this also seems ugly.
> >
> > It is the safest way, but the correct pseudo-code would be, assuiming
> > unsigned:
> >
> > when someone changes start:
> >
> > if (start > 99)
> > start = 99;
>
> I think we should just return -EINVAL in such cases. Allowing users to
> write larger percentages is a bit pointless (we don't allow them to write
> negative ones either). And other promiment code (the backlight drivers)
> seem to reject out-of-range values.
>
> > set_thresholds(start, stop);
>
> I think there should not be some common set_thresholds, because we also
> need to write things in different orders for start / stop then:
>
> DECREASE STOP => Write new start if needed, then write stop
> INCREASE START => Write new stop if needed, then write start
>
> Otherwise we might have a very very very short time in which start
> is greater than stop.
>
> I'll incorporate this in real code and test it tomorrow.
>

OK, tommorow is now, 4 months later. I was to busy with
lectures. An updated patch is below that passes my small
test suite.

The next step is to integrate this properly with power supply
and/or acpi battery. One way would be to add additional power
supply properties and then add get/set_property() pointers to
the acpi battery which it can fall back to if it does not support
a requested property (and we would locate the ACPI battery and
set those pointers to new thinkpad_acpi functions).

Full changelog:
* Changed stop interval to 1..100
* Fixed name of attributes (tresh => thresh)
* Made sure to keep start < stop

-- >8 --
Subject: [PATCH 1/4] thinkpad_acpi: Add support for controlling charge
thresholds

Add support for battery charge thresholds in new Sandy Bridge and Ivy Bridge
ThinkPads. Based on the unofficial documentation in tpacpi-bat.

Signed-off-by: Julian Andres Klode <j...@jak-linux.org>
---

Documentation/laptops/thinkpad-acpi.txt | 15 ++
drivers/platform/x86/thinkpad_acpi.c | 235 ++++++++++++++++++++++++++++++++
2 files changed, 250 insertions(+)

diff --git a/Documentation/laptops/thinkpad-acpi.txt b/Documentation/laptops/thinkpad-acpi.txt
index 86c5236..9b8a637 100644

--- a/Documentation/laptops/thinkpad-acpi.txt
+++ b/Documentation/laptops/thinkpad-acpi.txt
@@ -46,6 +46,7 @@ detailed description):
- Fan control and monitoring: fan speed, fan enable/disable
- WAN enable and disable
- UWB enable and disable
+ - Charging control

A compatibility table by model and feature is maintained on the web
site, http://ibm-acpi.sf.net/. I appreciate any success or failure

@@ -1352,6 +1353,20 @@ Sysfs notes:

rfkill controller switch "tpacpi_uwb_sw": refer to
Documentation/rfkill.txt for details.

+Charging control
+----------------
+sysfs attribute groups: BAT0, BAT1, and so on, depending on how many batteries
+are supported by the embedded controller.
+
+This feature controls the battery charging process.
+
+battery sysfs attribute: start_charge_thresh
+

+ A percentage value from 0-99%, controlling when charging should start

+
+battery sysfs attribute: stop_charge_thresh
+

+ A percentage value from 1-100%, controlling when charging should stop

Multiple Commands, Module Parameters
------------------------------------
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c

index 03ca6c1..fa29d82 100644

--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -323,6 +323,7 @@ static struct {
u32 sensors_pdrv_attrs_registered:1;
u32 sensors_pdev_attrs_registered:1;
u32 hotkey_poll_active:1;
+ u32 battery:1;
} tp_features;

static struct {

@@ -8350,6 +8351,236 @@ static struct ibm_struct fan_driver_data = {

.resume = fan_resume,
};

+
+/*************************************************************************
+ * Battery subdriver
+ */
+
+/* Modify battery_init() if you modify them */
+#define BATTERY_MAX_COUNT 3
+#define BATTERY_MAX_ATTRS 2
+
+static struct battery {
+ char name[3 + 1 + 1];
+ struct attribute_set *set;
+ struct dev_ext_attribute attributes[BATTERY_MAX_ATTRS];
+} batteries[BATTERY_MAX_COUNT];
+
+static int battery_attribute_get_battery(struct device_attribute *attr)
+{
+ return (int) (unsigned long) container_of(attr,
+ struct dev_ext_attribute,
+ attr)->var;
+}
+

+static int battery_read_threshold(int bat, char *threshold)
+{
+ int result = 0;
+
+ if (!hkey_handle || !acpi_evalf(hkey_handle, &result, threshold,
+ "dd", bat) || result < 0)
+ return -EIO;
+
+ /* Translate 0 to 100 for stop threshold */
+ if (strcmp(threshold, "BCSG") == 0 && (result & 0xFF) == 0)
+ return 100;
+ /* Bits 0 - 7 contain the current threshold */
+ return result & 0xFF;
+}
+
+static int battery_write_stop_charge_thresh(int bat, int value,
+ bool adjust_start);
+
+static int battery_write_start_charge_thresh(int bat, int value,
+ bool adjust_stop)
+{
+ int res = 0;
+
+ if (value < 0 || value > 99)
+ return -EINVAL;
+
+ /* Adjust the stop value if stop < new start value */
+ if (adjust_stop) {
+ int stop = battery_read_threshold(bat, "BCSG");
+ if (stop < 0)
+ return stop;
+ if (stop <= value)
+ res = battery_write_stop_charge_thresh(bat, value + 1,
+ FALSE);

+ if (res)
+ return res;
+ }
+

+ if (!hkey_handle || !acpi_evalf(hkey_handle, &res, "BCCS", "dd",

+ value | (bat << 8)) || res < 0)
+ return -EIO;
+

+ return 0;
+}
+

+static int battery_write_stop_charge_thresh(int bat, int value,
+ bool adjust_start)
+{
+ int res = 0;
+
+ if (value < 1 || value > 100)
+ return -EINVAL;
+
+ /* Adjust the start value if start > new stop value */
+ if (adjust_start) {
+ int start = battery_read_threshold(bat, "BCTG");
+ if (start < 0)
+ return start;
+ if (value != 0)
+ res = battery_write_start_charge_thresh(bat, value - 1,
+ FALSE);

+ if (res)
+ return res;
+ }
+

+ /* 0 means default which seems to be 100%. */
+ if (value == 100)
+ value = 0;

+
+ if (!hkey_handle || !acpi_evalf(hkey_handle, &res, "BCSS", "dd",

+ value | (bat << 8)) || res < 0)
+ return -EIO;
+

+ return 0;
+}
+

+static ssize_t battery_start_charge_thresh_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int bat = battery_attribute_get_battery(attr);
+ int res = -EINVAL;
+ unsigned long value;
+
+ res = kstrtoul(buf, 0, &value);

+ if (res)
+ return res;

+ res = battery_write_start_charge_thresh(bat, value, TRUE);
+ return res ? res : count;

+}
+
+static ssize_t battery_stop_charge_thresh_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int bat = battery_attribute_get_battery(attr);
+ int res = -EINVAL;
+ unsigned long value;
+
+ res = kstrtoul(buf, 0, &value);

+ if (res)
+ return res;

+ res = battery_write_stop_charge_thresh(bat, value, TRUE);
+ return res ? res : count;

+}
+
+static ssize_t battery_start_charge_thresh_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ int bat = battery_attribute_get_battery(attr);

+ int value = battery_read_threshold(bat, "BCTG");
+
+ return value < 0 ? value : snprintf(buf, PAGE_SIZE, "%d\n", value);

+}
+
+static ssize_t battery_stop_charge_thresh_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ int bat = battery_attribute_get_battery(attr);

+ int value = battery_read_threshold(bat, "BCSG");
+
+ return value < 0 ? value : snprintf(buf, PAGE_SIZE, "%d\n", value);

+ .attr = __ATTR(start_charge_thresh,

+ S_IWUSR | S_IRUGO,
+ battery_start_charge_thresh_show,
+ battery_start_charge_thresh_store),

+ .var = (void *)(unsigned long) (i + 1)

+ };
+ batteries[i].attributes[j++] = (struct dev_ext_attribute) {

+ .attr = __ATTR(stop_charge_thresh,

+ S_IWUSR | S_IRUGO,
+ battery_stop_charge_thresh_show,
+ battery_stop_charge_thresh_store),

+ .var = (void *)(unsigned long) (i + 1)

+ };
+
+ strncpy(batteries[i].name, "BAT", 3);
+ batteries[i].name[3] = '0' + i;
+ batteries[i].name[4] = '\0';

+ batteries[i].set = create_attr_set(j, batteries[i].name);

@@ -8741,6 +8972,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {

.data = &light_driver_data,
},
{
+ .init = battery_init,
+ .data = &battery_driver_data,
+ },
+ {
.init = cmos_init,
.data = &cmos_driver_data,
},
--

1.9.1

[Henrique de Moraes Holschuh]

On Sun, 06 Apr 2014, Julian Andres Klode wrote:
> OK, tommorow is now, 4 months later. I was to busy with
> lectures. An updated patch is below that passes my small
> test suite.
>
> The next step is to integrate this properly with power supply
> and/or acpi battery. One way would be to add additional power
> supply properties and then add get/set_property() pointers to
> the acpi battery which it can fall back to if it does not support
> a requested property (and we would locate the ACPI battery and
> set those pointers to new thinkpad_acpi functions).

Well, when you feel it is ready enough, please send a new version of the
entire changeset, with all changes (and label it v2 or something).

--
"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