thinkpad_acpi: add support for inhibit_charge

PATCH In review Ognjen Galic 4 participants 8 messages May 13, 2018 – May 14, 2018 2 messages not in the archive

Subsystems (3)

ACPI x86 platform drivers ACPI component architecture (acpica)

lore

Share & tools

lore

Ognjen Galic

author

2018-05-13 15:29 UTC #0 lore raw

PATCH 1/3 thinkpad_acpi: add support for inhibit_charge

Lenovo ThinkPad systems support the prevention of battery charging via a manual override called Inhibit Charge.

This patch adds support for that attribute and exposes it via the battery ACPI driver in the generic location:

/sys/class/power_supply/BATX/inhibit_charge

Signed-off-by Ognjen Galic <[email protected]>

---
 drivers/platform/x86/thinkpad_acpi.c | 66 +++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 2 deletions(-)

drivers/platform/x86/thinkpad_acpi.c +64 -2

index da1ca485..b8b74889 100644
@@ -9233,6 +9233,11 @@ static struct ibm_struct mute_led_driver_data = {
 #define GET_STOP	"BCSG"
 #define SET_STOP	"BCSS"
 
+#define SET_INHIBIT	"BICS"
+#define GET_INHIBIT	"BICG"
+
+#define INHIBIT_ATTR	"inhibit_charge"
+
 #define START_ATTR "charge_start_threshold"
 #define STOP_ATTR  "charge_stop_threshold"

@@ -9251,6 +9256,7 @@ enum {
 	/* This is used in the get/set helpers */
 	THRESHOLD_START,
 	THRESHOLD_STOP,
+	INHIBIT_CHARGE
 };
 
 struct tpacpi_battery_data {

@@ -9258,6 +9264,7 @@ struct tpacpi_battery_data {
 	int start_support;
 	int charge_stop;
 	int stop_support;
+	int inhibit_support;
 };
 
 struct tpacpi_battery_driver_data {

@@ -9315,6 +9322,13 @@ static int tpacpi_battery_get(int what, int battery, int *ret)
 		if (*ret == 0)
 			*ret = 100;
 		return 0;
+	case INHIBIT_CHARGE:
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, ret, battery))
+			return -ENODEV;
+
+		/* The inhibit charge status is in the first bit */
+		*ret = *ret & 0x01;
+		return 0;
 	default:
 		pr_crit("wrong parameter: %d", what);
 		return -EINVAL;

@@ -9343,6 +9357,21 @@ static int tpacpi_battery_set(int what, int battery, int value)
 			return -ENODEV;
 		}
 		return 0;
+	case INHIBIT_CHARGE:
+		/* When setting inhbitit charge, we set a default vaulue of
+		 * always breaking on AC detach and the effective time is set to
+		 * be permanent.
+		 * The battery ID is in bits 4-5, 2 bits and the effective time
+		 * is in bits 8-23, 2 bytes. A time of FFFF indicates forever.
+		 */
+		param = value;
+		param |= battery << 4;
+		param |= 0xFFFF << 8;
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(SET_INHIBIT, &ret, param)) {
+			pr_err("failed to set inhibit charge on %d", battery);
+			return -ENODEV;
+		}
+		return 0;
 	default:
 		pr_crit("wrong parameter: %d", what);
 		return -EINVAL;

@@ -9359,6 +9388,8 @@ static int tpacpi_battery_probe(int battery)
 	 * 2) Check for support
 	 * 3) Get the current stop threshold
 	 * 4) Check for support
+	 * 5) Check for inhibit charge support
+	 * 6) Get the current inhibit charge status
 	 */
 	if (acpi_has_method(hkey_handle, GET_START)) {
 		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_START, &ret, battery)) {

@@ -9395,10 +9426,16 @@ static int tpacpi_battery_probe(int battery)
 			return -ENODEV;
 		}
 	}
-	pr_info("battery %d registered (start %d, stop %d)",
+	if (acpi_has_method(hkey_handle, GET_INHIBIT))
+		if (!ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, &ret, battery)))
+			/* Support is marked in bit 5 */
+			battery_info.batteries[battery].inhibit_support = ret & BIT(5);
+
+	pr_info("battery %d registered (start %d, stop %d, inhibit: %d)",
 			battery,
 			battery_info.batteries[battery].charge_start,
-			battery_info.batteries[battery].charge_stop);
+			battery_info.batteries[battery].charge_stop,
+			battery_info.batteries[battery].inhibit_support);
 
 	return 0;
 }

@@ -9484,6 +9521,15 @@ static ssize_t tpacpi_battery_store(int what,
 		if (tpacpi_battery_set(THRESHOLD_STOP, battery, value))
 			return -EINVAL;
 		return count;
+	case INHIBIT_CHARGE:
+		if (!battery_info.batteries[battery].inhibit_support)
+			return -ENODEV;
+		/* The only valid values are 1 and 0 */
+		if (value != 0 && value != 1)
+			return -EINVAL;
+		if (tpacpi_battery_set(INHIBIT_CHARGE, battery, value))
+			return -ENODEV;
+		return count;
 	default:
 		pr_crit("Wrong parameter: %d", what);
 		return -EINVAL;

@@ -9546,12 +9592,28 @@ static ssize_t charge_stop_threshold_store(struct device *dev,
 	return tpacpi_battery_store(THRESHOLD_STOP, dev, buf, count);
 }
 
+static ssize_t inhibit_charge_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	return tpacpi_battery_store(INHIBIT_CHARGE, dev, buf, count);
+}
+
+static ssize_t inhibit_charge_show(struct device *device,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return tpacpi_battery_show(INHIBIT_CHARGE, device, buf);
+}
+
 static DEVICE_ATTR_RW(charge_start_threshold);
 static DEVICE_ATTR_RW(charge_stop_threshold);
+static DEVICE_ATTR_RW(inhibit_charge);
 
 static struct attribute *tpacpi_battery_attrs[] = {
 	&dev_attr_charge_start_threshold.attr,
 	&dev_attr_charge_stop_threshold.attr,
+	&dev_attr_inhibit_charge.attr,
 	NULL,
 };

Signature · 6 lines

2.17.0


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Christoph Böhmwalder

2018-05-14 09:46 UTC #1 lore raw

Quoted 60 lines · Ognjen Galic

Lenovo ThinkPad systems support the prevention of battery charging via a manual override called Inhibit Charge.

This patch adds support for that attribute and exposes it via the battery ACPI driver in the generic location:

/sys/class/power_supply/BATX/inhibit_charge

Signed-off-by Ognjen Galic <[email protected]>

---
 drivers/platform/x86/thinkpad_acpi.c | 66 +++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 2 deletions(-)

drivers/platform/x86/thinkpad_acpi.c +14 -0

index da1ca485..b8b74889 100644
@@ -9233,6 +9233,11 @@ static struct ibm_struct mute_led_driver_data = {
 #define GET_STOP	"BCSG"
 #define SET_STOP	"BCSS"
 
+#define SET_INHIBIT	"BICS"
+#define GET_INHIBIT	"BICG"
+
+#define INHIBIT_ATTR	"inhibit_charge"
+
 #define START_ATTR "charge_start_threshold"
 #define STOP_ATTR  "charge_stop_threshold"

@@ -9251,6 +9256,7 @@ enum {
 	/* This is used in the get/set helpers */
 	THRESHOLD_START,
 	THRESHOLD_STOP,
+	INHIBIT_CHARGE
 };
 
 struct tpacpi_battery_data {

@@ -9258,6 +9264,7 @@ struct tpacpi_battery_data {
 	int start_support;
 	int charge_stop;
 	int stop_support;
+	int inhibit_support;
 };
 
 struct tpacpi_battery_driver_data {

@@ -9315,6 +9322,13 @@ static int tpacpi_battery_get(int what, int battery, int *ret)
 		if (*ret == 0)
 			*ret = 100;
 		return 0;
+	case INHIBIT_CHARGE:
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, ret, battery))
+			return -ENODEV;
+
+		/* The inhibit charge status is in the first bit */
+		*ret = *ret & 0x01;
+		return 0;
 	default:
 		pr_crit("wrong parameter: %d", what);
 		return -EINVAL;

drivers/platform/x86/thinkpad_acpi.c static int tpacpi_battery_set(int what, int battery, int value) · line 9361

Quoted 5 lines

drivers/platform/x86/thinkpad_acpi.c +2 -0

@@ -9343,6 +9357,21 @@ static int tpacpi_battery_set(int what, int battery, int value)
 			return -ENODEV;
 		}
 		return 0;
+	case INHIBIT_CHARGE:
+		/* When setting inhbitit charge, we set a default vaulue of

This comment does not adhere to the Linux coding style

Quoted 45 lines

drivers/platform/x86/thinkpad_acpi.c +23 -2

+		 * always breaking on AC detach and the effective time is set to
+		 * be permanent.
+		 * The battery ID is in bits 4-5, 2 bits and the effective time
+		 * is in bits 8-23, 2 bytes. A time of FFFF indicates forever.
+		 */
+		param = value;
+		param |= battery << 4;
+		param |= 0xFFFF << 8;
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(SET_INHIBIT, &ret, param)) {
+			pr_err("failed to set inhibit charge on %d", battery);
+			return -ENODEV;
+		}
+		return 0;
 	default:
 		pr_crit("wrong parameter: %d", what);
 		return -EINVAL;

@@ -9359,6 +9388,8 @@ static int tpacpi_battery_probe(int battery)
 	 * 2) Check for support
 	 * 3) Get the current stop threshold
 	 * 4) Check for support
+	 * 5) Check for inhibit charge support
+	 * 6) Get the current inhibit charge status
 	 */
 	if (acpi_has_method(hkey_handle, GET_START)) {
 		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_START, &ret, battery)) {

@@ -9395,10 +9426,16 @@ static int tpacpi_battery_probe(int battery)
 			return -ENODEV;
 		}
 	}
-	pr_info("battery %d registered (start %d, stop %d)",
+	if (acpi_has_method(hkey_handle, GET_INHIBIT))
+		if (!ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, &ret, battery)))
+			/* Support is marked in bit 5 */
+			battery_info.batteries[battery].inhibit_support = ret & BIT(5);
+
+	pr_info("battery %d registered (start %d, stop %d, inhibit: %d)",
 			battery,
 			battery_info.batteries[battery].charge_start,
-			battery_info.batteries[battery].charge_stop);
+			battery_info.batteries[battery].charge_stop,
+			battery_info.batteries[battery].inhibit_support);
 
 	return 0;
 }

drivers/platform/x86/thinkpad_acpi.c static ssize_t tpacpi_battery_store(int what, · line 9528

Quoted 8 lines

drivers/platform/x86/thinkpad_acpi.c +5 -0

@@ -9484,6 +9521,15 @@ static ssize_t tpacpi_battery_store(int what,
 		if (tpacpi_battery_set(THRESHOLD_STOP, battery, value))
 			return -EINVAL;
 		return count;
+	case INHIBIT_CHARGE:
+		if (!battery_info.batteries[battery].inhibit_support)
+			return -ENODEV;
+		/* The only valid values are 1 and 0 */
+		if (value != 0 && value != 1)

I'm not sure, but maybe if (value < 2) is better here?

Quoted 39 lines

drivers/platform/x86/thinkpad_acpi.c +20 -0

+			return -EINVAL;
+		if (tpacpi_battery_set(INHIBIT_CHARGE, battery, value))
+			return -ENODEV;
+		return count;
 	default:
 		pr_crit("Wrong parameter: %d", what);
 		return -EINVAL;

@@ -9546,12 +9592,28 @@ static ssize_t charge_stop_threshold_store(struct device *dev,
 	return tpacpi_battery_store(THRESHOLD_STOP, dev, buf, count);
 }
 
+static ssize_t inhibit_charge_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	return tpacpi_battery_store(INHIBIT_CHARGE, dev, buf, count);
+}
+
+static ssize_t inhibit_charge_show(struct device *device,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return tpacpi_battery_show(INHIBIT_CHARGE, device, buf);
+}
+
 static DEVICE_ATTR_RW(charge_start_threshold);
 static DEVICE_ATTR_RW(charge_stop_threshold);
+static DEVICE_ATTR_RW(inhibit_charge);
 
 static struct attribute *tpacpi_battery_attrs[] = {
 	&dev_attr_charge_start_threshold.attr,
 	&dev_attr_charge_stop_threshold.attr,
+	&dev_attr_inhibit_charge.attr,
 	NULL,
 };

Signature · 1 line

2.17.0

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Andy Shevchenko

2018-05-14 10:41 UTC #2 lore raw

Quoted 4 lines · Christoph Böhmwalder

Quoted 3 lines · Ognjen Galic
Lenovo ThinkPad systems support the prevention of battery charging via a manual override called Inhibit Charge.

Quoted 8 lines

Quoted 6 lines
This patch adds support for that attribute and exposes it via the battery ACPI driver in the generic location:
/sys/class/power_supply/BATX/inhibit_charge
+ /* When setting inhbitit charge, we set a default vaulue of
This comment does not adhere to the Linux coding style

While you are right in principle, the whole driver is so old and uses this style. So, for such cases we, as maintainers, prefer less deviation work, i.e. keeping the
style is a good thing to do.

Quoted 4 lines

drivers/platform/x86/thinkpad_acpi.c static ssize_t tpacpi_battery_store(int what, · line 9528
Quoted 2 lines
drivers/platform/x86/thinkpad_acpi.c +2 -0
+             /* The only valid values are 1 and 0 */
+             if (value != 0 && value != 1)
I'm not sure, but maybe if (value < 2) is better here?

Since it's about integer-as-a-boolean, test for bit 0 would be sufficient, i.e. ~BIT(0). Though, I find this form not so readable since the input comes actually from the user.
It would be nice to have just kstrtobool() called instead for such options, but see above. It would need a (huge) refactoring of the driver first.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

A message that is not in the archive (sent off-list or not yet received): CAHp75VdPOH7J1X4ZXmRsQ1yrB5n5RRmhPneWOnu7TFOAysgE6Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Ognjen Galić

author

2018-05-14 13:38 UTC #4 lore raw

Quoted 11 lines · Andy Shevchenko

Quoted 8 lines · Christoph Böhmwalder
Quoted 7 lines · Ognjen Galic
Lenovo ThinkPad systems support the prevention of battery charging via a manual override called Inhibit Charge.
This patch adds support for that attribute and exposes it via the battery ACPI driver in the generic location:
/sys/class/power_supply/BATX/inhibit_charge

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
+ /* When setting inhbitit charge, we set a default vaulue of
This comment does not adhere to the Linux coding style
While you are right in principle, the whole driver is so old and uses this style. So, for such cases we, as maintainers, prefer less deviation work, i.e. keeping the
style is a good thing to do.

That's what I did, follow the driver style.

Quoted 8 lines

Quoted 5 lines
drivers/platform/x86/thinkpad_acpi.c static ssize_t tpacpi_battery_store(int what, · line 9528
Quoted 3 lines
drivers/platform/x86/thinkpad_acpi.c +2 -0
+             /* The only valid values are 1 and 0 */
+             if (value != 0 && value != 1)
I'm not sure, but maybe if (value < 2) is better here?
Since it's about integer-as-a-boolean, test for bit 0 would be sufficient, i.e. ~BIT(0).

That seems uncessarily complicated. Whats wrong with LT and GT operators?

Quoted 2 lines

Though, I find this form not so readable
since the input comes actually from the user.

I agree.

Quoted 3 lines

It would be nice to have just kstrtobool() called instead for such options, but see above. It would need a (huge) refactoring of the driver first.

That needs a whole lot of refactoring for no real functional benefit.

Quoted 4 lines

Signature · 3 lines
-- 
With Best Regards,
Andy Shevchenko

Henrique de Moraes Holschuh

2018-05-14 11:39 UTC #5 lore raw

In reply to Christoph Böhmwalder

Quoted 7 lines · Christoph Böhmwalder

drivers/platform/x86/thinkpad_acpi.c static int tpacpi_battery_get(int what, int battery, int *ret) · line 9331
Quoted 7 lines
drivers/platform/x86/thinkpad_acpi.c +7 -0
+	case INHIBIT_CHARGE:
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, ret, battery))
+			return -ENODEV;
+
+		/* The inhibit charge status is in the first bit */
+		*ret = *ret & 0x01;
+		return 0;

Do we know what is in the other bits? If so, please document the ACPI method using a comment somewhere in the driver code, like you did for SET_INHIBIT.

Quoted 11 lines

drivers/platform/x86/thinkpad_acpi.c static int tpacpi_battery_set(int what, int battery, int value) · line 9361
Quoted 9 lines
drivers/platform/x86/thinkpad_acpi.c +2 -0
 	default:
 		pr_crit("wrong parameter: %d", what);
 		return -EINVAL;
@@ -9343,6 +9357,21 @@ static int tpacpi_battery_set(int what, int battery, int value)
 			return -ENODEV;
 		}
 		return 0;
+	case INHIBIT_CHARGE:
+		/* When setting inhbitit charge, we set a default vaulue of
This comment does not adhere to the Linux coding style

Much on the driver doesn't, because it is _OLD_. But yeah, it is preferrable to fix this as we add code, so it would be good to have all new (and modified) comments switched to modern kernel style.

Quoted 7 lines

drivers/platform/x86/thinkpad_acpi.c static ssize_t tpacpi_battery_store(int what, · line 9528
Quoted 5 lines
drivers/platform/x86/thinkpad_acpi.c +5 -0
+	case INHIBIT_CHARGE:
+		if (!battery_info.batteries[battery].inhibit_support)
+			return -ENODEV;
+		/* The only valid values are 1 and 0 */
+		if (value != 0 && value != 1)
I'm not sure, but maybe if (value < 2) is better here?

Indeed... with a comment that says 0 = main battery, 1 = extra/dock battery or something.

Signature · 5 lines

  Henrique Holschuh

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

A message that is not in the archive (sent off-list or not yet received): 20180514113928.dnsxjqov7l6rvorb-ZGHd14iZgfaRjzvQDGKj+xxZW9W5cXbT@public.gmane.org

Ognjen Galić

author

2018-05-14 13:40 UTC #7 lore raw

Quoted 12 lines · Henrique de Moraes Holschuh

Quoted 7 lines · Christoph Böhmwalder
drivers/platform/x86/thinkpad_acpi.c static int tpacpi_battery_get(int what, int battery, int *ret) · line 9331
Quoted 7 lines
drivers/platform/x86/thinkpad_acpi.c +7 -0
+	case INHIBIT_CHARGE:
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, ret, battery))
+			return -ENODEV;
+
+		/* The inhibit charge status is in the first bit */
+		*ret = *ret & 0x01;
+		return 0;
Do we know what is in the other bits? If so, please document the ACPI method using a comment somewhere in the driver code, like you did for SET_INHIBIT.

I got the specs for the methods in a Lenovo doc I was told not to share around, including information in it.

Quoted 13 lines

Quoted 12 lines
drivers/platform/x86/thinkpad_acpi.c static int tpacpi_battery_set(int what, int battery, int value) · line 9361
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +2 -0
 	default:
 		pr_crit("wrong parameter: %d", what);
 		return -EINVAL;
@@ -9343,6 +9357,21 @@ static int tpacpi_battery_set(int what, int battery, int value)
 			return -ENODEV;
 		}
 		return 0;
+	case INHIBIT_CHARGE:
+		/* When setting inhbitit charge, we set a default vaulue of
This comment does not adhere to the Linux coding style

Quoted 3 lines

Much on the driver doesn't, because it is _OLD_. But yeah, it is preferrable to fix this as we add code, so it would be good to have all new (and modified) comments switched to modern kernel style.

So what do you people want me to do? Should I fix the comments or leave as-is?

Quoted 11 lines

Quoted 8 lines
drivers/platform/x86/thinkpad_acpi.c static ssize_t tpacpi_battery_store(int what, · line 9528
Quoted 6 lines
drivers/platform/x86/thinkpad_acpi.c +5 -0
+	case INHIBIT_CHARGE:
+		if (!battery_info.batteries[battery].inhibit_support)
+			return -ENODEV;
+		/* The only valid values are 1 and 0 */
+		if (value != 0 && value != 1)
I'm not sure, but maybe if (value < 2) is better here?
Indeed... with a comment that says 0 = main battery, 1 = extra/dock battery or something.

That seems like obfuscation to me, this way its clear that it must be either 1 or 0. And inhibiting is set per-battery so 1 is on and not battery 1.

Quoted 3 lines

Signature · 2 lines
-- 
  Henrique Holschuh

Henrique de Moraes Holschuh

2018-05-14 15:41 UTC #8 lore raw

Quoted 14 lines · Ognjen Galić

Quoted 12 lines · Henrique de Moraes Holschuh
Quoted 7 lines · Christoph Böhmwalder
drivers/platform/x86/thinkpad_acpi.c static int tpacpi_battery_get(int what, int battery, int *ret) · line 9331
Quoted 7 lines
drivers/platform/x86/thinkpad_acpi.c +7 -0
+	case INHIBIT_CHARGE:
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, ret, battery))
+			return -ENODEV;
+
+		/* The inhibit charge status is in the first bit */
+		*ret = *ret & 0x01;
+		return 0;
Do we know what is in the other bits? If so, please document the ACPI method using a comment somewhere in the driver code, like you did for SET_INHIBIT.

Quoted 2 lines

I got the specs for the methods in a Lenovo doc I was told not to share around, including information in it.

Hmm, ok. Please send to me in private the email address of the person I should ask for the documentation since you cannot distribute the documentation itself. This isn't the first time it happened.

The usual rule I follow is: document what is either already being used in the driver or which very likely wil have to be used in the driver. Unrelated stuff (e.g. for some unrelated functionality the driver does not implement) doesn't have to be documented.

In this case, we need to at least know what bits need to be RMW, or always-zero when calling SET_* methods...

Quoted 2 lines

So what do you people want me to do? Should I fix the comments or leave as-is?

Since the subsystem maintainer said it should be kept as-is, and I don't much care, please keep it as-is. It won't matter for my driver maintainer ack :-)

Quoted 8 lines

Quoted 8 lines
Quoted 7 lines
drivers/platform/x86/thinkpad_acpi.c static ssize_t tpacpi_battery_store(int what, · line 9528
Quoted 5 lines
drivers/platform/x86/thinkpad_acpi.c +5 -0
+	case INHIBIT_CHARGE:
+		if (!battery_info.batteries[battery].inhibit_support)
+			return -ENODEV;
+		/* The only valid values are 1 and 0 */
+		if (value != 0 && value != 1)
I'm not sure, but maybe if (value < 2) is better here?

Quoted 6 lines

Quoted 2 lines
Indeed... with a comment that says 0 = main battery, 1 = extra/dock battery or something.
That seems like obfuscation to me, this way its clear that it must be either 1 or 0. And inhibiting is set per-battery so 1 is on and not battery 1.

Hmm, yes I got that wrong. "battery" should use 0 <= battery <2 for checking (if needed), but value can only be 0 or 1.

I'd personally have used a bitmask test for "value", ensuring all other bits are zero, though. But that's just a matter of taste and I don't think any of the choices (including the one you used) are "bad taste", so all of them are fine as far as I am concerned.

Signature · 9 lines

  Henrique Holschuh

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
ibm-acpi-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Ognjen Galić

author

2018-05-14 13:36 UTC #9 lore raw

In reply to Christoph Böhmwalder

Quoted 60 lines · Christoph Böhmwalder

Quoted 59 lines · Ognjen Galic

Lenovo ThinkPad systems support the prevention of battery charging via a manual override called Inhibit Charge.

This patch adds support for that attribute and exposes it via the battery ACPI driver in the generic location:

/sys/class/power_supply/BATX/inhibit_charge

Signed-off-by Ognjen Galic <[email protected]>

---
 drivers/platform/x86/thinkpad_acpi.c | 66 +++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 2 deletions(-)

drivers/platform/x86/thinkpad_acpi.c +14 -0

index da1ca485..b8b74889 100644
@@ -9233,6 +9233,11 @@ static struct ibm_struct mute_led_driver_data = {
 #define GET_STOP	"BCSG"
 #define SET_STOP	"BCSS"
 
+#define SET_INHIBIT	"BICS"
+#define GET_INHIBIT	"BICG"
+
+#define INHIBIT_ATTR	"inhibit_charge"
+
 #define START_ATTR "charge_start_threshold"
 #define STOP_ATTR  "charge_stop_threshold"

@@ -9251,6 +9256,7 @@ enum {
 	/* This is used in the get/set helpers */
 	THRESHOLD_START,
 	THRESHOLD_STOP,
+	INHIBIT_CHARGE
 };
 
 struct tpacpi_battery_data {

@@ -9258,6 +9264,7 @@ struct tpacpi_battery_data {
 	int start_support;
 	int charge_stop;
 	int stop_support;
+	int inhibit_support;
 };
 
 struct tpacpi_battery_driver_data {

@@ -9315,6 +9322,13 @@ static int tpacpi_battery_get(int what, int battery, int *ret)
 		if (*ret == 0)
 			*ret = 100;
 		return 0;
+	case INHIBIT_CHARGE:
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, ret, battery))
+			return -ENODEV;
+
+		/* The inhibit charge status is in the first bit */
+		*ret = *ret & 0x01;
+		return 0;
 	default:
 		pr_crit("wrong parameter: %d", what);
 		return -EINVAL;

Quoted 8 lines

drivers/platform/x86/thinkpad_acpi.c static int tpacpi_battery_set(int what, int battery, int value) · line 9361
Quoted 6 lines
drivers/platform/x86/thinkpad_acpi.c +2 -0
@@ -9343,6 +9357,21 @@ static int tpacpi_battery_set(int what, int battery, int value)
 			return -ENODEV;
 		}
 		return 0;
+	case INHIBIT_CHARGE:
+		/* When setting inhbitit charge, we set a default vaulue of
This comment does not adhere to the Linux coding style

I followed the style in the driver.

Quoted 43 lines

drivers/platform/x86/thinkpad_acpi.c +23 -2

+		 * always breaking on AC detach and the effective time is set to
+		 * be permanent.
+		 * The battery ID is in bits 4-5, 2 bits and the effective time
+		 * is in bits 8-23, 2 bytes. A time of FFFF indicates forever.
+		 */
+		param = value;
+		param |= battery << 4;
+		param |= 0xFFFF << 8;
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(SET_INHIBIT, &ret, param)) {
+			pr_err("failed to set inhibit charge on %d", battery);
+			return -ENODEV;
+		}
+		return 0;
 	default:
 		pr_crit("wrong parameter: %d", what);
 		return -EINVAL;

@@ -9359,6 +9388,8 @@ static int tpacpi_battery_probe(int battery)
 	 * 2) Check for support
 	 * 3) Get the current stop threshold
 	 * 4) Check for support
+	 * 5) Check for inhibit charge support
+	 * 6) Get the current inhibit charge status
 	 */
 	if (acpi_has_method(hkey_handle, GET_START)) {
 		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_START, &ret, battery)) {

@@ -9395,10 +9426,16 @@ static int tpacpi_battery_probe(int battery)
 			return -ENODEV;
 		}
 	}
-	pr_info("battery %d registered (start %d, stop %d)",
+	if (acpi_has_method(hkey_handle, GET_INHIBIT))
+		if (!ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_INHIBIT, &ret, battery)))
+			/* Support is marked in bit 5 */
+			battery_info.batteries[battery].inhibit_support = ret & BIT(5);
+
+	pr_info("battery %d registered (start %d, stop %d, inhibit: %d)",
 			battery,
 			battery_info.batteries[battery].charge_start,
-			battery_info.batteries[battery].charge_stop);
+			battery_info.batteries[battery].charge_stop,
+			battery_info.batteries[battery].inhibit_support);

Quoted 13 lines

drivers/platform/x86/thinkpad_acpi.c static ssize_t tpacpi_battery_store(int what, · line 9528
Quoted 11 lines
drivers/platform/x86/thinkpad_acpi.c +5 -0
 	return 0;
 }
@@ -9484,6 +9521,15 @@ static ssize_t tpacpi_battery_store(int what,
 		if (tpacpi_battery_set(THRESHOLD_STOP, battery, value))
 			return -EINVAL;
 		return count;
+	case INHIBIT_CHARGE:
+		if (!battery_info.batteries[battery].inhibit_support)
+			return -ENODEV;
+		/* The only valid values are 1 and 0 */
+		if (value != 0 && value != 1)
I'm not sure, but maybe if (value < 2) is better here?

I think the exisiting validation is just fine. Anything other is cosmetic and obfuscation IMO.

Quoted 37 lines

drivers/platform/x86/thinkpad_acpi.c +20 -0

+			return -EINVAL;
+		if (tpacpi_battery_set(INHIBIT_CHARGE, battery, value))
+			return -ENODEV;
+		return count;
 	default:
 		pr_crit("Wrong parameter: %d", what);
 		return -EINVAL;

@@ -9546,12 +9592,28 @@ static ssize_t charge_stop_threshold_store(struct device *dev,
 	return tpacpi_battery_store(THRESHOLD_STOP, dev, buf, count);
 }
 
+static ssize_t inhibit_charge_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	return tpacpi_battery_store(INHIBIT_CHARGE, dev, buf, count);
+}
+
+static ssize_t inhibit_charge_show(struct device *device,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return tpacpi_battery_show(INHIBIT_CHARGE, device, buf);
+}
+
 static DEVICE_ATTR_RW(charge_start_threshold);
 static DEVICE_ATTR_RW(charge_stop_threshold);
+static DEVICE_ATTR_RW(inhibit_charge);
 
 static struct attribute *tpacpi_battery_attrs[] = {
 	&dev_attr_charge_start_threshold.attr,
 	&dev_attr_charge_stop_threshold.attr,
+	&dev_attr_inhibit_charge.attr,
 	NULL,
 };

Quoted 3 lines

Quoted 3 lines
Signature · 1 line
2.17.0