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@xxxxxxxxx>
> > ---
> > 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
> >
