| Message ID | 20180513152937.GA5072@thinkpad (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
On Sun, May 13, 2018 at 05:29:37PM +0200, Ognjen Galic wrote: > 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 <smclt30p@gmail.com> > --- > drivers/platform/x86/thinkpad_acpi.c | 66 +++++++++++++++++++++++++++- > 1 file changed, 64 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index da1ca485..b8b74889 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -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 This comment does not adhere to the Linux coding style > + * 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) I'm not sure, but maybe `if (value < 2)` is better here? > + 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, > }; > > -- > 2.17.0 >
On Mon, May 14, 2018 at 12:46 PM, Christoph Böhmwalder <christoph@boehmwalder.at> wrote: > On Sun, May 13, 2018 at 05:29:37PM +0200, Ognjen Galic wrote: >> 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 >> + /* 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. >> + /* 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.
On Mon, 14 May 2018, Christoph Böhmwalder wrote: > > + 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. > > 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. > > + 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.
On Mon, May 14, 2018 at 11:46:20AM +0200, Christoph Böhmwalder wrote: > On Sun, May 13, 2018 at 05:29:37PM +0200, Ognjen Galic wrote: > > 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 <smclt30p@gmail.com> > > --- > > drivers/platform/x86/thinkpad_acpi.c | 66 +++++++++++++++++++++++++++- > > 1 file changed, 64 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > > index da1ca485..b8b74889 100644 > > --- a/drivers/platform/x86/thinkpad_acpi.c > > +++ b/drivers/platform/x86/thinkpad_acpi.c > > @@ -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 > > This comment does not adhere to the Linux coding style I followed the style in the driver. > > > + * 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) > > 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. > > > + 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, > > }; > > > > -- > > 2.17.0 > >
On Mon, May 14, 2018 at 01:41:33PM +0300, Andy Shevchenko wrote: > On Mon, May 14, 2018 at 12:46 PM, Christoph Böhmwalder > <christoph@boehmwalder.at> wrote: > > On Sun, May 13, 2018 at 05:29:37PM +0200, Ognjen Galic wrote: > >> 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 > > >> + /* 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. > > >> + /* 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? > Though, I find this form not so readable > since the input comes actually from the user. I agree. > 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. > > -- > With Best Regards, > Andy Shevchenko
On Mon, May 14, 2018 at 08:39:28AM -0300, Henrique de Moraes Holschuh wrote: > On Mon, 14 May 2018, Christoph Böhmwalder wrote: > > > + 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. > > > > 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. So what do you people want me to do? Should I fix the comments or leave as-is? > > > > + 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. > > -- > Henrique Holschuh
On Mon, 14 May 2018, Ognjen Galić wrote: > On Mon, May 14, 2018 at 08:39:28AM -0300, Henrique de Moraes Holschuh wrote: > > On Mon, 14 May 2018, Christoph Böhmwalder wrote: > > > > + 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. 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... > 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 :-) > > > > + 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. 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.
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index da1ca485..b8b74889 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -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, };
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 <smclt30p@gmail.com> --- drivers/platform/x86/thinkpad_acpi.c | 66 +++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 2 deletions(-)