diff mbox

[v13,3/4] thinkpad_acpi: Add support for battery thresholds

Message ID 20180207145844.bd567c36juvlkjvz@thinkpad (mailing list archive)
State Awaiting Upstream, archived
Delegated to: Andy Shevchenko
Headers show

Commit Message

Ognjen Galic Feb. 7, 2018, 2:58 p.m. UTC
1) Charge start threshold
/sys/class/power_supply/BATN/charge_start_threshold

Valid values are [0, 99]. A value of 0 turns off the
start threshold wear control.

2) Charge stop threshold
/sys/class/power_supply/BATN/charge_stop_threshold

Valid values are [1, 100]. A value of 100 turns off
the stop threshold wear control. This must be
configured first.

Signed-off-by: Ognjen Galic <smclt30p@gmail.com>
---

Notes:
    v2:
    * Re-write the patch to make the changes in
    battery.c generic as suggested by Rafael
    
    v3:
    * Fixed a bug where you could not set the stop
    threshold to 100% when the start threshold is 0%
    * Fixed the "Not Charging" quirk to match Lenovo's
    BIOS Firmware specification
    
    v4:
    * Fixed a bug where you could not set the start
    threshold to >stop if stop was 100%
    
    v5:
    * Migrated from symbol_get to native linking,
    to fix module dependencies
    * Fixed a bug where unloading the module would
    cause a BUG inside battery
    * Fixed a bug where you could unload battery
    before unloading thinkpad_acpi
    
    v6:
    * Fixed all the style and naming issues pointed
    out by Andy Shevchenko
    
    v7:
    * No changes in this patch in v7
    
    v8:
    * No changes in this patch in v8
    
    v9:
    * Use DEVICE_ATTR_RW instead of DEVICE_ATTR
    * Use bitopts.h instead of raw operators
    * Remove redundant whitespaces
    * Remove redundant comments
    * Move the power_supply changes to separate patch
    * Fix other various styling issues pointed out by
    Andy Shevchenko
    
    v10:
    * Fix a pasting error from v6 that prevents the setting
    of the start threshold with EINVAL
    
    v11:
    * Fix formatting of changelog
    
    v12:
    * Remove most whitespaces between ifs
    * Change int to acpi_status in tpacpi_battery_acpi_eval
    
    v13:
    * Change if (!tpacpi_... to if ACPI_FAILURE(...

 drivers/platform/x86/Kconfig         |   1 +
 drivers/platform/x86/thinkpad_acpi.c | 389 ++++++++++++++++++++++++++++++++++-
 2 files changed, 389 insertions(+), 1 deletion(-)

Comments

Rafael J. Wysocki Feb. 8, 2018, 3:33 p.m. UTC | #1
On Wed, Feb 7, 2018 at 3:58 PM, Ognjen Galic <smclt30p@gmail.com> wrote:
> 1) Charge start threshold
> /sys/class/power_supply/BATN/charge_start_threshold
>
> Valid values are [0, 99]. A value of 0 turns off the
> start threshold wear control.
>
> 2) Charge stop threshold
> /sys/class/power_supply/BATN/charge_stop_threshold
>
> Valid values are [1, 100]. A value of 100 turns off
> the stop threshold wear control. This must be
> configured first.
>
> Signed-off-by: Ognjen Galic <smclt30p@gmail.com>

Andy, Darren, Henrique,

I'm assuming no concerns about this you from you, please let me know
if that's not correct.

> ---
>
> Notes:
>     v2:
>     * Re-write the patch to make the changes in
>     battery.c generic as suggested by Rafael
>
>     v3:
>     * Fixed a bug where you could not set the stop
>     threshold to 100% when the start threshold is 0%
>     * Fixed the "Not Charging" quirk to match Lenovo's
>     BIOS Firmware specification
>
>     v4:
>     * Fixed a bug where you could not set the start
>     threshold to >stop if stop was 100%
>
>     v5:
>     * Migrated from symbol_get to native linking,
>     to fix module dependencies
>     * Fixed a bug where unloading the module would
>     cause a BUG inside battery
>     * Fixed a bug where you could unload battery
>     before unloading thinkpad_acpi
>
>     v6:
>     * Fixed all the style and naming issues pointed
>     out by Andy Shevchenko
>
>     v7:
>     * No changes in this patch in v7
>
>     v8:
>     * No changes in this patch in v8
>
>     v9:
>     * Use DEVICE_ATTR_RW instead of DEVICE_ATTR
>     * Use bitopts.h instead of raw operators
>     * Remove redundant whitespaces
>     * Remove redundant comments
>     * Move the power_supply changes to separate patch
>     * Fix other various styling issues pointed out by
>     Andy Shevchenko
>
>     v10:
>     * Fix a pasting error from v6 that prevents the setting
>     of the start threshold with EINVAL
>
>     v11:
>     * Fix formatting of changelog
>
>     v12:
>     * Remove most whitespaces between ifs
>     * Change int to acpi_status in tpacpi_battery_acpi_eval
>
>     v13:
>     * Change if (!tpacpi_... to if ACPI_FAILURE(...
>
>  drivers/platform/x86/Kconfig         |   1 +
>  drivers/platform/x86/thinkpad_acpi.c | 389 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 389 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 9a8f96465..0d13d30c1 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -425,6 +425,7 @@ config SURFACE3_WMI
>  config THINKPAD_ACPI
>         tristate "ThinkPad ACPI Laptop Extras"
>         depends on ACPI
> +       depends on ACPI_BATTERY
>         depends on INPUT
>         depends on RFKILL || RFKILL = n
>         depends on ACPI_VIDEO || ACPI_VIDEO = n
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index d5eaf3b1e..1c57ee2b6 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -23,7 +23,7 @@
>
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> -#define TPACPI_VERSION "0.25"
> +#define TPACPI_VERSION "0.26"
>  #define TPACPI_SYSFS_VERSION 0x030000
>
>  /*
> @@ -66,6 +66,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/sysfs.h>
>  #include <linux/backlight.h>
> +#include <linux/bitops.h>
>  #include <linux/fb.h>
>  #include <linux/platform_device.h>
>  #include <linux/hwmon.h>
> @@ -78,11 +79,13 @@
>  #include <linux/workqueue.h>
>  #include <linux/acpi.h>
>  #include <linux/pci_ids.h>
> +#include <linux/power_supply.h>
>  #include <linux/thinkpad_acpi.h>
>  #include <sound/core.h>
>  #include <sound/control.h>
>  #include <sound/initval.h>
>  #include <linux/uaccess.h>
> +#include <acpi/battery.h>
>  #include <acpi/video.h>
>
>  /* ThinkPad CMOS commands */
> @@ -335,6 +338,7 @@ static struct {
>         u32 sensors_pdev_attrs_registered:1;
>         u32 hotkey_poll_active:1;
>         u32 has_adaptive_kbd:1;
> +       u32 battery:1;
>  } tp_features;
>
>  static struct {
> @@ -9209,6 +9213,385 @@ static struct ibm_struct mute_led_driver_data = {
>         .resume = mute_led_resume,
>  };
>
> +/*
> + * Battery Wear Control Driver
> + * Contact: Ognjen Galic <smclt30p@gmail.com>
> + */
> +
> +/* Metadata */
> +
> +#define GET_START      "BCTG"
> +#define SET_START      "BCCS"
> +#define GET_STOP       "BCSG"
> +#define SET_STOP       "BCSS"
> +
> +#define START_ATTR "charge_start_threshold"
> +#define STOP_ATTR  "charge_stop_threshold"
> +
> +enum {
> +       BAT_ANY = 0,
> +       BAT_PRIMARY = 1,
> +       BAT_SECONDARY = 2
> +};
> +
> +enum {
> +       /* Error condition bit */
> +       METHOD_ERR = BIT(31),
> +};
> +
> +enum {
> +       /* This is used in the get/set helpers */
> +       THRESHOLD_START,
> +       THRESHOLD_STOP,
> +};
> +
> +struct tpacpi_battery_data {
> +       int charge_start;
> +       int start_support;
> +       int charge_stop;
> +       int stop_support;
> +};
> +
> +struct tpacpi_battery_driver_data {
> +       struct tpacpi_battery_data batteries[3];
> +       int individual_addressing;
> +};
> +
> +static struct tpacpi_battery_driver_data battery_info;
> +
> +/* ACPI helpers/functions/probes */
> +
> +/**
> + * This evaluates a ACPI method call specific to the battery
> + * ACPI extension. The specifics are that an error is marked
> + * in the 32rd bit of the response, so we just check that here.
> + */
> +static acpi_status tpacpi_battery_acpi_eval(char *method, int *ret, int param)
> +{
> +       int response;
> +
> +       if (!acpi_evalf(hkey_handle, &response, method, "dd", param)) {
> +               acpi_handle_err(hkey_handle, "%s: evaluate failed", method);
> +               return AE_ERROR;
> +       }
> +       if (response & METHOD_ERR) {
> +               acpi_handle_err(hkey_handle,
> +                               "%s evaluated but flagged as error", method);
> +               return AE_ERROR;
> +       }
> +       *ret = response;
> +       return AE_OK;
> +}
> +
> +static int tpacpi_battery_get(int what, int battery, int *ret)
> +{
> +       switch (what) {
> +       case THRESHOLD_START:
> +               if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_START, ret, battery))
> +                       return -ENODEV;
> +
> +               /* The value is in the low 8 bits of the response */
> +               *ret = *ret & 0xFF;
> +               return 0;
> +       case THRESHOLD_STOP:
> +               if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_STOP, ret, battery))
> +                       return -ENODEV;
> +               /* Value is in lower 8 bits */
> +               *ret = *ret & 0xFF;
> +               /*
> +                * On the stop value, if we return 0 that
> +                * does not make any sense. 0 means Default, which
> +                * means that charging stops at 100%, so we return
> +                * that.
> +                */
> +               if (*ret == 0)
> +                       *ret = 100;
> +               return 0;
> +       default:
> +               pr_crit("wrong parameter: %d", what);
> +               return -EINVAL;
> +       }
> +}
> +
> +static int tpacpi_battery_set(int what, int battery, int value)
> +{
> +       int param, ret;
> +       /* The first 8 bits are the value of the threshold */
> +       param = value;
> +       /* The battery ID is in bits 8-9, 2 bits */
> +       param |= battery << 8;
> +
> +       switch (what) {
> +       case THRESHOLD_START:
> +               if ACPI_FAILURE(tpacpi_battery_acpi_eval(SET_START, &ret, param)) {
> +                       pr_err("failed to set charge threshold on battery %d",
> +                                       battery);
> +                       return -ENODEV;
> +               }
> +               return 0;
> +       case THRESHOLD_STOP:
> +               if ACPI_FAILURE(tpacpi_battery_acpi_eval(SET_STOP, &ret, param)) {
> +                       pr_err("failed to set stop threshold: %d", battery);
> +                       return -ENODEV;
> +               }
> +               return 0;
> +       default:
> +               pr_crit("wrong parameter: %d", what);
> +               return -EINVAL;
> +       }
> +}
> +
> +static int tpacpi_battery_probe(int battery)
> +{
> +       int ret = 0;
> +
> +       memset(&battery_info, 0, sizeof(struct tpacpi_battery_driver_data));
> +       /*
> +        * 1) Get the current start threshold
> +        * 2) Check for support
> +        * 3) Get the current stop threshold
> +        * 4) Check for support
> +        */
> +       if (acpi_has_method(hkey_handle, GET_START)) {
> +               if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_START, &ret, battery)) {
> +                       pr_err("Error probing battery %d\n", battery);
> +                       return -ENODEV;
> +               }
> +               /* Individual addressing is in bit 9 */
> +               if (ret & BIT(9))
> +                       battery_info.individual_addressing = true;
> +               /* Support is marked in bit 8 */
> +               if (ret & BIT(8))
> +                       battery_info.batteries[battery].start_support = 1;
> +               else
> +                       return -ENODEV;
> +               if (tpacpi_battery_get(THRESHOLD_START, battery,
> +                       &battery_info.batteries[battery].charge_start)) {
> +                       pr_err("Error probing battery %d\n", battery);
> +                       return -ENODEV;
> +               }
> +       }
> +       if (acpi_has_method(hkey_handle, GET_STOP)) {
> +               if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_STOP, &ret, battery)) {
> +                       pr_err("Error probing battery stop; %d\n", battery);
> +                       return -ENODEV;
> +               }
> +               /* Support is marked in bit 8 */
> +               if (ret & BIT(8))
> +                       battery_info.batteries[battery].stop_support = 1;
> +               else
> +                       return -ENODEV;
> +               if (tpacpi_battery_get(THRESHOLD_STOP, battery,
> +                       &battery_info.batteries[battery].charge_stop)) {
> +                       pr_err("Error probing battery stop: %d\n", battery);
> +                       return -ENODEV;
> +               }
> +       }
> +       pr_info("battery %d registered (start %d, stop %d)",
> +                       battery,
> +                       battery_info.batteries[battery].charge_start,
> +                       battery_info.batteries[battery].charge_stop);
> +
> +       return 0;
> +}
> +
> +/* General helper functions */
> +
> +static int tpacpi_battery_get_id(const char *battery_name)
> +{
> +
> +       if (strcmp(battery_name, "BAT0") == 0)
> +               return BAT_PRIMARY;
> +       if (strcmp(battery_name, "BAT1") == 0)
> +               return BAT_SECONDARY;
> +       /*
> +        * If for some reason the battery is not BAT0 nor is it
> +        * BAT1, we will assume it's the default, first battery,
> +        * AKA primary.
> +        */
> +       pr_warn("unknown battery %s, assuming primary", battery_name);
> +       return BAT_PRIMARY;
> +}
> +
> +/* sysfs interface */
> +
> +static ssize_t tpacpi_battery_store(int what,
> +                                   struct device *dev,
> +                                   const char *buf, size_t count)
> +{
> +       struct power_supply *supply = to_power_supply(dev);
> +       unsigned long value;
> +       int battery, rval;
> +       /*
> +        * Some systems have support for more than
> +        * one battery. If that is the case,
> +        * tpacpi_battery_probe marked that addressing
> +        * them individually is supported, so we do that
> +        * based on the device struct.
> +        *
> +        * On systems that are not supported, we assume
> +        * the primary as most of the ACPI calls fail
> +        * with "Any Battery" as the parameter.
> +        */
> +       if (battery_info.individual_addressing)
> +               /* BAT_PRIMARY or BAT_SECONDARY */
> +               battery = tpacpi_battery_get_id(supply->desc->name);
> +       else
> +               battery = BAT_PRIMARY;
> +
> +       rval = kstrtoul(buf, 10, &value);
> +       if (rval)
> +               return rval;
> +
> +       switch (what) {
> +       case THRESHOLD_START:
> +               if (!battery_info.batteries[battery].start_support)
> +                       return -ENODEV;
> +               /* valid values are [0, 99] */
> +               if (value < 0 || value > 99)
> +                       return -EINVAL;
> +               if (value > battery_info.batteries[battery].charge_stop)
> +                       return -EINVAL;
> +               if (tpacpi_battery_set(THRESHOLD_START, battery, value))
> +                       return -ENODEV;
> +               battery_info.batteries[battery].charge_start = value;
> +               return count;
> +
> +       case THRESHOLD_STOP:
> +               if (!battery_info.batteries[battery].stop_support)
> +                       return -ENODEV;
> +               /* valid values are [1, 100] */
> +               if (value < 1 || value > 100)
> +                       return -EINVAL;
> +               if (value < battery_info.batteries[battery].charge_start)
> +                       return -EINVAL;
> +               battery_info.batteries[battery].charge_stop = value;
> +               /*
> +                * When 100 is passed to stop, we need to flip
> +                * it to 0 as that the EC understands that as
> +                * "Default", which will charge to 100%
> +                */
> +               if (value == 100)
> +                       value = 0;
> +               if (tpacpi_battery_set(THRESHOLD_STOP, battery, value))
> +                       return -EINVAL;
> +               return count;
> +       default:
> +               pr_crit("Wrong parameter: %d", what);
> +               return -EINVAL;
> +       }
> +       return count;
> +}
> +
> +static ssize_t tpacpi_battery_show(int what,
> +                                  struct device *dev,
> +                                  char *buf)
> +{
> +       struct power_supply *supply = to_power_supply(dev);
> +       int ret, battery;
> +       /*
> +        * Some systems have support for more than
> +        * one battery. If that is the case,
> +        * tpacpi_battery_probe marked that addressing
> +        * them individually is supported, so we;
> +        * based on the device struct.
> +        *
> +        * On systems that are not supported, we assume
> +        * the primary as most of the ACPI calls fail
> +        * with "Any Battery" as the parameter.
> +        */
> +       if (battery_info.individual_addressing)
> +               /* BAT_PRIMARY or BAT_SECONDARY */
> +               battery = tpacpi_battery_get_id(supply->desc->name);
> +       else
> +               battery = BAT_PRIMARY;
> +       if (tpacpi_battery_get(what, battery, &ret))
> +               return -ENODEV;
> +       return sprintf(buf, "%d\n", ret);
> +}
> +
> +static ssize_t charge_start_threshold_show(struct device *device,
> +                               struct device_attribute *attr,
> +                               char *buf)
> +{
> +       return tpacpi_battery_show(THRESHOLD_START, device, buf);
> +}
> +
> +static ssize_t charge_stop_threshold_show(struct device *device,
> +                               struct device_attribute *attr,
> +                               char *buf)
> +{
> +       return tpacpi_battery_show(THRESHOLD_STOP, device, buf);
> +}
> +
> +static ssize_t charge_start_threshold_store(struct device *dev,
> +                               struct device_attribute *attr,
> +                               const char *buf, size_t count)
> +{
> +       return tpacpi_battery_store(THRESHOLD_START, dev, buf, count);
> +}
> +
> +static ssize_t charge_stop_threshold_store(struct device *dev,
> +                               struct device_attribute *attr,
> +                               const char *buf, size_t count)
> +{
> +       return tpacpi_battery_store(THRESHOLD_STOP, dev, buf, count);
> +}
> +
> +static DEVICE_ATTR_RW(charge_start_threshold);
> +static DEVICE_ATTR_RW(charge_stop_threshold);
> +
> +static struct attribute *tpacpi_battery_attrs[] = {
> +       &dev_attr_charge_start_threshold.attr,
> +       &dev_attr_charge_stop_threshold.attr,
> +       NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(tpacpi_battery);
> +
> +/* ACPI battery hooking */
> +
> +static int tpacpi_battery_add(struct power_supply *battery)
> +{
> +       int batteryid = tpacpi_battery_get_id(battery->desc->name);
> +
> +       if (tpacpi_battery_probe(batteryid))
> +               return -ENODEV;
> +       if (device_add_groups(&battery->dev, tpacpi_battery_groups))
> +               return -ENODEV;
> +       return 0;
> +}
> +
> +static int tpacpi_battery_remove(struct power_supply *battery)
> +{
> +       device_remove_groups(&battery->dev, tpacpi_battery_groups);
> +       return 0;
> +}
> +
> +static struct acpi_battery_hook battery_hook = {
> +       .add_battery = tpacpi_battery_add,
> +       .remove_battery = tpacpi_battery_remove,
> +       .name = "ThinkPad Battery Extension",
> +};
> +
> +/* Subdriver init/exit */
> +
> +static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
> +{
> +       battery_hook_register(&battery_hook);
> +       return 0;
> +}
> +
> +static void tpacpi_battery_exit(void)
> +{
> +       battery_hook_unregister(&battery_hook);
> +}
> +
> +static struct ibm_struct battery_driver_data = {
> +       .name = "battery",
> +       .exit = tpacpi_battery_exit,
> +};
> +
>  /****************************************************************************
>   ****************************************************************************
>   *
> @@ -9655,6 +10038,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>                 .init = mute_led_init,
>                 .data = &mute_led_driver_data,
>         },
> +       {
> +               .init = tpacpi_battery_init,
> +               .data = &battery_driver_data,
> +       },
>  };
>
>  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
> --
> 2.14.1
>
Sebastian Reichel Feb. 8, 2018, 10:03 p.m. UTC | #2
Hi,

On Wed, Feb 07, 2018 at 03:58:44PM +0100, Ognjen Galic wrote:
> 1) Charge start threshold
> /sys/class/power_supply/BATN/charge_start_threshold
> 
> Valid values are [0, 99]. A value of 0 turns off the
> start threshold wear control.
> 
> 2) Charge stop threshold
> /sys/class/power_supply/BATN/charge_stop_threshold
> 
> Valid values are [1, 100]. A value of 100 turns off
> the stop threshold wear control. This must be
> configured first.

This is a new sysfs file, that should be documented. Also this
looks pretty generic. Just introduce new POWER_SUPPLY_PROP_
entries for start/stop charging threshold and use them.

-- Sebastian

> Signed-off-by: Ognjen Galic <smclt30p@gmail.com>
> ---
> 
> Notes:
>     v2:
>     * Re-write the patch to make the changes in
>     battery.c generic as suggested by Rafael
>     
>     v3:
>     * Fixed a bug where you could not set the stop
>     threshold to 100% when the start threshold is 0%
>     * Fixed the "Not Charging" quirk to match Lenovo's
>     BIOS Firmware specification
>     
>     v4:
>     * Fixed a bug where you could not set the start
>     threshold to >stop if stop was 100%
>     
>     v5:
>     * Migrated from symbol_get to native linking,
>     to fix module dependencies
>     * Fixed a bug where unloading the module would
>     cause a BUG inside battery
>     * Fixed a bug where you could unload battery
>     before unloading thinkpad_acpi
>     
>     v6:
>     * Fixed all the style and naming issues pointed
>     out by Andy Shevchenko
>     
>     v7:
>     * No changes in this patch in v7
>     
>     v8:
>     * No changes in this patch in v8
>     
>     v9:
>     * Use DEVICE_ATTR_RW instead of DEVICE_ATTR
>     * Use bitopts.h instead of raw operators
>     * Remove redundant whitespaces
>     * Remove redundant comments
>     * Move the power_supply changes to separate patch
>     * Fix other various styling issues pointed out by
>     Andy Shevchenko
>     
>     v10:
>     * Fix a pasting error from v6 that prevents the setting
>     of the start threshold with EINVAL
>     
>     v11:
>     * Fix formatting of changelog
>     
>     v12:
>     * Remove most whitespaces between ifs
>     * Change int to acpi_status in tpacpi_battery_acpi_eval
>     
>     v13:
>     * Change if (!tpacpi_... to if ACPI_FAILURE(...
> 
>  drivers/platform/x86/Kconfig         |   1 +
>  drivers/platform/x86/thinkpad_acpi.c | 389 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 389 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 9a8f96465..0d13d30c1 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -425,6 +425,7 @@ config SURFACE3_WMI
>  config THINKPAD_ACPI
>  	tristate "ThinkPad ACPI Laptop Extras"
>  	depends on ACPI
> +	depends on ACPI_BATTERY
>  	depends on INPUT
>  	depends on RFKILL || RFKILL = n
>  	depends on ACPI_VIDEO || ACPI_VIDEO = n
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index d5eaf3b1e..1c57ee2b6 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -23,7 +23,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> -#define TPACPI_VERSION "0.25"
> +#define TPACPI_VERSION "0.26"
>  #define TPACPI_SYSFS_VERSION 0x030000
>  
>  /*
> @@ -66,6 +66,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/sysfs.h>
>  #include <linux/backlight.h>
> +#include <linux/bitops.h>
>  #include <linux/fb.h>
>  #include <linux/platform_device.h>
>  #include <linux/hwmon.h>
> @@ -78,11 +79,13 @@
>  #include <linux/workqueue.h>
>  #include <linux/acpi.h>
>  #include <linux/pci_ids.h>
> +#include <linux/power_supply.h>
>  #include <linux/thinkpad_acpi.h>
>  #include <sound/core.h>
>  #include <sound/control.h>
>  #include <sound/initval.h>
>  #include <linux/uaccess.h>
> +#include <acpi/battery.h>
>  #include <acpi/video.h>
>  
>  /* ThinkPad CMOS commands */
> @@ -335,6 +338,7 @@ static struct {
>  	u32 sensors_pdev_attrs_registered:1;
>  	u32 hotkey_poll_active:1;
>  	u32 has_adaptive_kbd:1;
> +	u32 battery:1;
>  } tp_features;
>  
>  static struct {
> @@ -9209,6 +9213,385 @@ static struct ibm_struct mute_led_driver_data = {
>  	.resume = mute_led_resume,
>  };
>  
> +/*
> + * Battery Wear Control Driver
> + * Contact: Ognjen Galic <smclt30p@gmail.com>
> + */
> +
> +/* Metadata */
> +
> +#define GET_START	"BCTG"
> +#define SET_START	"BCCS"
> +#define GET_STOP	"BCSG"
> +#define SET_STOP	"BCSS"
> +
> +#define START_ATTR "charge_start_threshold"
> +#define STOP_ATTR  "charge_stop_threshold"
> +
> +enum {
> +	BAT_ANY = 0,
> +	BAT_PRIMARY = 1,
> +	BAT_SECONDARY = 2
> +};
> +
> +enum {
> +	/* Error condition bit */
> +	METHOD_ERR = BIT(31),
> +};
> +
> +enum {
> +	/* This is used in the get/set helpers */
> +	THRESHOLD_START,
> +	THRESHOLD_STOP,
> +};
> +
> +struct tpacpi_battery_data {
> +	int charge_start;
> +	int start_support;
> +	int charge_stop;
> +	int stop_support;
> +};
> +
> +struct tpacpi_battery_driver_data {
> +	struct tpacpi_battery_data batteries[3];
> +	int individual_addressing;
> +};
> +
> +static struct tpacpi_battery_driver_data battery_info;
> +
> +/* ACPI helpers/functions/probes */
> +
> +/**
> + * This evaluates a ACPI method call specific to the battery
> + * ACPI extension. The specifics are that an error is marked
> + * in the 32rd bit of the response, so we just check that here.
> + */
> +static acpi_status tpacpi_battery_acpi_eval(char *method, int *ret, int param)
> +{
> +	int response;
> +
> +	if (!acpi_evalf(hkey_handle, &response, method, "dd", param)) {
> +		acpi_handle_err(hkey_handle, "%s: evaluate failed", method);
> +		return AE_ERROR;
> +	}
> +	if (response & METHOD_ERR) {
> +		acpi_handle_err(hkey_handle,
> +				"%s evaluated but flagged as error", method);
> +		return AE_ERROR;
> +	}
> +	*ret = response;
> +	return AE_OK;
> +}
> +
> +static int tpacpi_battery_get(int what, int battery, int *ret)
> +{
> +	switch (what) {
> +	case THRESHOLD_START:
> +		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_START, ret, battery))
> +			return -ENODEV;
> +
> +		/* The value is in the low 8 bits of the response */
> +		*ret = *ret & 0xFF;
> +		return 0;
> +	case THRESHOLD_STOP:
> +		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_STOP, ret, battery))
> +			return -ENODEV;
> +		/* Value is in lower 8 bits */
> +		*ret = *ret & 0xFF;
> +		/*
> +		 * On the stop value, if we return 0 that
> +		 * does not make any sense. 0 means Default, which
> +		 * means that charging stops at 100%, so we return
> +		 * that.
> +		 */
> +		if (*ret == 0)
> +			*ret = 100;
> +		return 0;
> +	default:
> +		pr_crit("wrong parameter: %d", what);
> +		return -EINVAL;
> +	}
> +}
> +
> +static int tpacpi_battery_set(int what, int battery, int value)
> +{
> +	int param, ret;
> +	/* The first 8 bits are the value of the threshold */
> +	param = value;
> +	/* The battery ID is in bits 8-9, 2 bits */
> +	param |= battery << 8;
> +
> +	switch (what) {
> +	case THRESHOLD_START:
> +		if ACPI_FAILURE(tpacpi_battery_acpi_eval(SET_START, &ret, param)) {
> +			pr_err("failed to set charge threshold on battery %d",
> +					battery);
> +			return -ENODEV;
> +		}
> +		return 0;
> +	case THRESHOLD_STOP:
> +		if ACPI_FAILURE(tpacpi_battery_acpi_eval(SET_STOP, &ret, param)) {
> +			pr_err("failed to set stop threshold: %d", battery);
> +			return -ENODEV;
> +		}
> +		return 0;
> +	default:
> +		pr_crit("wrong parameter: %d", what);
> +		return -EINVAL;
> +	}
> +}
> +
> +static int tpacpi_battery_probe(int battery)
> +{
> +	int ret = 0;
> +
> +	memset(&battery_info, 0, sizeof(struct tpacpi_battery_driver_data));
> +	/*
> +	 * 1) Get the current start threshold
> +	 * 2) Check for support
> +	 * 3) Get the current stop threshold
> +	 * 4) Check for support
> +	 */
> +	if (acpi_has_method(hkey_handle, GET_START)) {
> +		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_START, &ret, battery)) {
> +			pr_err("Error probing battery %d\n", battery);
> +			return -ENODEV;
> +		}
> +		/* Individual addressing is in bit 9 */
> +		if (ret & BIT(9))
> +			battery_info.individual_addressing = true;
> +		/* Support is marked in bit 8 */
> +		if (ret & BIT(8))
> +			battery_info.batteries[battery].start_support = 1;
> +		else
> +			return -ENODEV;
> +		if (tpacpi_battery_get(THRESHOLD_START, battery,
> +			&battery_info.batteries[battery].charge_start)) {
> +			pr_err("Error probing battery %d\n", battery);
> +			return -ENODEV;
> +		}
> +	}
> +	if (acpi_has_method(hkey_handle, GET_STOP)) {
> +		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_STOP, &ret, battery)) {
> +			pr_err("Error probing battery stop; %d\n", battery);
> +			return -ENODEV;
> +		}
> +		/* Support is marked in bit 8 */
> +		if (ret & BIT(8))
> +			battery_info.batteries[battery].stop_support = 1;
> +		else
> +			return -ENODEV;
> +		if (tpacpi_battery_get(THRESHOLD_STOP, battery,
> +			&battery_info.batteries[battery].charge_stop)) {
> +			pr_err("Error probing battery stop: %d\n", battery);
> +			return -ENODEV;
> +		}
> +	}
> +	pr_info("battery %d registered (start %d, stop %d)",
> +			battery,
> +			battery_info.batteries[battery].charge_start,
> +			battery_info.batteries[battery].charge_stop);
> +
> +	return 0;
> +}
> +
> +/* General helper functions */
> +
> +static int tpacpi_battery_get_id(const char *battery_name)
> +{
> +
> +	if (strcmp(battery_name, "BAT0") == 0)
> +		return BAT_PRIMARY;
> +	if (strcmp(battery_name, "BAT1") == 0)
> +		return BAT_SECONDARY;
> +	/*
> +	 * If for some reason the battery is not BAT0 nor is it
> +	 * BAT1, we will assume it's the default, first battery,
> +	 * AKA primary.
> +	 */
> +	pr_warn("unknown battery %s, assuming primary", battery_name);
> +	return BAT_PRIMARY;
> +}
> +
> +/* sysfs interface */
> +
> +static ssize_t tpacpi_battery_store(int what,
> +				    struct device *dev,
> +				    const char *buf, size_t count)
> +{
> +	struct power_supply *supply = to_power_supply(dev);
> +	unsigned long value;
> +	int battery, rval;
> +	/*
> +	 * Some systems have support for more than
> +	 * one battery. If that is the case,
> +	 * tpacpi_battery_probe marked that addressing
> +	 * them individually is supported, so we do that
> +	 * based on the device struct.
> +	 *
> +	 * On systems that are not supported, we assume
> +	 * the primary as most of the ACPI calls fail
> +	 * with "Any Battery" as the parameter.
> +	 */
> +	if (battery_info.individual_addressing)
> +		/* BAT_PRIMARY or BAT_SECONDARY */
> +		battery = tpacpi_battery_get_id(supply->desc->name);
> +	else
> +		battery = BAT_PRIMARY;
> +
> +	rval = kstrtoul(buf, 10, &value);
> +	if (rval)
> +		return rval;
> +
> +	switch (what) {
> +	case THRESHOLD_START:
> +		if (!battery_info.batteries[battery].start_support)
> +			return -ENODEV;
> +		/* valid values are [0, 99] */
> +		if (value < 0 || value > 99)
> +			return -EINVAL;
> +		if (value > battery_info.batteries[battery].charge_stop)
> +			return -EINVAL;
> +		if (tpacpi_battery_set(THRESHOLD_START, battery, value))
> +			return -ENODEV;
> +		battery_info.batteries[battery].charge_start = value;
> +		return count;
> +
> +	case THRESHOLD_STOP:
> +		if (!battery_info.batteries[battery].stop_support)
> +			return -ENODEV;
> +		/* valid values are [1, 100] */
> +		if (value < 1 || value > 100)
> +			return -EINVAL;
> +		if (value < battery_info.batteries[battery].charge_start)
> +			return -EINVAL;
> +		battery_info.batteries[battery].charge_stop = value;
> +		/*
> +		 * When 100 is passed to stop, we need to flip
> +		 * it to 0 as that the EC understands that as
> +		 * "Default", which will charge to 100%
> +		 */
> +		if (value == 100)
> +			value = 0;
> +		if (tpacpi_battery_set(THRESHOLD_STOP, battery, value))
> +			return -EINVAL;
> +		return count;
> +	default:
> +		pr_crit("Wrong parameter: %d", what);
> +		return -EINVAL;
> +	}
> +	return count;
> +}
> +
> +static ssize_t tpacpi_battery_show(int what,
> +				   struct device *dev,
> +				   char *buf)
> +{
> +	struct power_supply *supply = to_power_supply(dev);
> +	int ret, battery;
> +	/*
> +	 * Some systems have support for more than
> +	 * one battery. If that is the case,
> +	 * tpacpi_battery_probe marked that addressing
> +	 * them individually is supported, so we;
> +	 * based on the device struct.
> +	 *
> +	 * On systems that are not supported, we assume
> +	 * the primary as most of the ACPI calls fail
> +	 * with "Any Battery" as the parameter.
> +	 */
> +	if (battery_info.individual_addressing)
> +		/* BAT_PRIMARY or BAT_SECONDARY */
> +		battery = tpacpi_battery_get_id(supply->desc->name);
> +	else
> +		battery = BAT_PRIMARY;
> +	if (tpacpi_battery_get(what, battery, &ret))
> +		return -ENODEV;
> +	return sprintf(buf, "%d\n", ret);
> +}
> +
> +static ssize_t charge_start_threshold_show(struct device *device,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	return tpacpi_battery_show(THRESHOLD_START, device, buf);
> +}
> +
> +static ssize_t charge_stop_threshold_show(struct device *device,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	return tpacpi_battery_show(THRESHOLD_STOP, device, buf);
> +}
> +
> +static ssize_t charge_start_threshold_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	return tpacpi_battery_store(THRESHOLD_START, dev, buf, count);
> +}
> +
> +static ssize_t charge_stop_threshold_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	return tpacpi_battery_store(THRESHOLD_STOP, dev, buf, count);
> +}
> +
> +static DEVICE_ATTR_RW(charge_start_threshold);
> +static DEVICE_ATTR_RW(charge_stop_threshold);
> +
> +static struct attribute *tpacpi_battery_attrs[] = {
> +	&dev_attr_charge_start_threshold.attr,
> +	&dev_attr_charge_stop_threshold.attr,
> +	NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(tpacpi_battery);
> +
> +/* ACPI battery hooking */
> +
> +static int tpacpi_battery_add(struct power_supply *battery)
> +{
> +	int batteryid = tpacpi_battery_get_id(battery->desc->name);
> +
> +	if (tpacpi_battery_probe(batteryid))
> +		return -ENODEV;
> +	if (device_add_groups(&battery->dev, tpacpi_battery_groups))
> +		return -ENODEV;
> +	return 0;
> +}
> +
> +static int tpacpi_battery_remove(struct power_supply *battery)
> +{
> +	device_remove_groups(&battery->dev, tpacpi_battery_groups);
> +	return 0;
> +}
> +
> +static struct acpi_battery_hook battery_hook = {
> +	.add_battery = tpacpi_battery_add,
> +	.remove_battery = tpacpi_battery_remove,
> +	.name = "ThinkPad Battery Extension",
> +};
> +
> +/* Subdriver init/exit */
> +
> +static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
> +{
> +	battery_hook_register(&battery_hook);
> +	return 0;
> +}
> +
> +static void tpacpi_battery_exit(void)
> +{
> +	battery_hook_unregister(&battery_hook);
> +}
> +
> +static struct ibm_struct battery_driver_data = {
> +	.name = "battery",
> +	.exit = tpacpi_battery_exit,
> +};
> +
>  /****************************************************************************
>   ****************************************************************************
>   *
> @@ -9655,6 +10038,10 @@ static struct ibm_init_struct ibms_init[] __initdata = {
>  		.init = mute_led_init,
>  		.data = &mute_led_driver_data,
>  	},
> +	{
> +		.init = tpacpi_battery_init,
> +		.data = &battery_driver_data,
> +	},
>  };
>  
>  static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
> -- 
> 2.14.1
>
Rafael J. Wysocki Feb. 9, 2018, 9:22 a.m. UTC | #3
On Thu, Feb 8, 2018 at 11:03 PM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Wed, Feb 07, 2018 at 03:58:44PM +0100, Ognjen Galic wrote:
>> 1) Charge start threshold
>> /sys/class/power_supply/BATN/charge_start_threshold
>>
>> Valid values are [0, 99]. A value of 0 turns off the
>> start threshold wear control.
>>
>> 2) Charge stop threshold
>> /sys/class/power_supply/BATN/charge_stop_threshold
>>
>> Valid values are [1, 100]. A value of 100 turns off
>> the stop threshold wear control. This must be
>> configured first.
>
> This is a new sysfs file, that should be documented.

Right, I should have remembered about that, sorry.

> Also this looks pretty generic. Just introduce new POWER_SUPPLY_PROP_
> entries for start/stop charging threshold and use them.

What about doing this as a follow-up?
Henrique de Moraes Holschuh Feb. 9, 2018, 10:24 a.m. UTC | #4
Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Sebastian Reichel Feb. 9, 2018, 10:39 a.m. UTC | #5
Hi,

On Fri, Feb 09, 2018 at 10:22:54AM +0100, Rafael J. Wysocki wrote:
> On Thu, Feb 8, 2018 at 11:03 PM, Sebastian Reichel <sre@kernel.org> wrote:
> > Hi,
> >
> > On Wed, Feb 07, 2018 at 03:58:44PM +0100, Ognjen Galic wrote:
> >> 1) Charge start threshold
> >> /sys/class/power_supply/BATN/charge_start_threshold
> >>
> >> Valid values are [0, 99]. A value of 0 turns off the
> >> start threshold wear control.
> >>
> >> 2) Charge stop threshold
> >> /sys/class/power_supply/BATN/charge_stop_threshold
> >>
> >> Valid values are [1, 100]. A value of 100 turns off
> >> the stop threshold wear control. This must be
> >> configured first.
> >
> > This is a new sysfs file, that should be documented.
> 
> Right, I should have remembered about that, sorry.
> 
> > Also this looks pretty generic. Just introduce new POWER_SUPPLY_PROP_
> > entries for start/stop charging threshold and use them.
> 
> What about doing this as a follow-up?

Fine with me.

-- Sebastian
Rafael J. Wysocki Feb. 9, 2018, 10:49 a.m. UTC | #6
On Fri, Feb 9, 2018 at 11:39 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Fri, Feb 09, 2018 at 10:22:54AM +0100, Rafael J. Wysocki wrote:
>> On Thu, Feb 8, 2018 at 11:03 PM, Sebastian Reichel <sre@kernel.org> wrote:
>> > Hi,
>> >
>> > On Wed, Feb 07, 2018 at 03:58:44PM +0100, Ognjen Galic wrote:
>> >> 1) Charge start threshold
>> >> /sys/class/power_supply/BATN/charge_start_threshold
>> >>
>> >> Valid values are [0, 99]. A value of 0 turns off the
>> >> start threshold wear control.
>> >>
>> >> 2) Charge stop threshold
>> >> /sys/class/power_supply/BATN/charge_stop_threshold
>> >>
>> >> Valid values are [1, 100]. A value of 100 turns off
>> >> the stop threshold wear control. This must be
>> >> configured first.
>> >
>> > This is a new sysfs file, that should be documented.
>>
>> Right, I should have remembered about that, sorry.
>>
>> > Also this looks pretty generic. Just introduce new POWER_SUPPLY_PROP_
>> > entries for start/stop charging threshold and use them.
>>
>> What about doing this as a follow-up?
>
> Fine with me.

OK

Actually, I don't see any documentation whatever for ACPI battery and
AC power supply properties, so I guess that needs to be added in
general and I don't think it would be fair to ask Ognjen to do that in
order to get the extension in.

Why don't we pencil this in as work to do?
Sebastian Reichel Feb. 9, 2018, 12:34 p.m. UTC | #7
Hi,

On Fri, Feb 09, 2018 at 11:49:40AM +0100, Rafael J. Wysocki wrote:
> On Fri, Feb 9, 2018 at 11:39 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > Hi,
> >
> > On Fri, Feb 09, 2018 at 10:22:54AM +0100, Rafael J. Wysocki wrote:
> >> On Thu, Feb 8, 2018 at 11:03 PM, Sebastian Reichel <sre@kernel.org> wrote:
> >> > Hi,
> >> >
> >> > On Wed, Feb 07, 2018 at 03:58:44PM +0100, Ognjen Galic wrote:
> >> >> 1) Charge start threshold
> >> >> /sys/class/power_supply/BATN/charge_start_threshold
> >> >>
> >> >> Valid values are [0, 99]. A value of 0 turns off the
> >> >> start threshold wear control.
> >> >>
> >> >> 2) Charge stop threshold
> >> >> /sys/class/power_supply/BATN/charge_stop_threshold
> >> >>
> >> >> Valid values are [1, 100]. A value of 100 turns off
> >> >> the stop threshold wear control. This must be
> >> >> configured first.
> >> >
> >> > This is a new sysfs file, that should be documented.
> >>
> >> Right, I should have remembered about that, sorry.
> >>
> >> > Also this looks pretty generic. Just introduce new POWER_SUPPLY_PROP_
> >> > entries for start/stop charging threshold and use them.
> >>
> >> What about doing this as a follow-up?
> >
> > Fine with me.
> 
> OK
> 
> Actually, I don't see any documentation whatever for ACPI battery and
> AC power supply properties, so I guess that needs to be added in
> general and I don't think it would be fair to ask Ognjen to do that in
> order to get the extension in.
> 
> Why don't we pencil this in as work to do?

The generic ones are documented here:

Documentation/power/power_supply_class.txt

-- Sebastian
Ognjen Galic Feb. 10, 2018, 8:48 a.m. UTC | #8
On Fri, Feb 09, 2018 at 01:34:14PM +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Feb 09, 2018 at 11:49:40AM +0100, Rafael J. Wysocki wrote:
> > On Fri, Feb 9, 2018 at 11:39 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > > Hi,
> > >
> > > On Fri, Feb 09, 2018 at 10:22:54AM +0100, Rafael J. Wysocki wrote:
> > >> On Thu, Feb 8, 2018 at 11:03 PM, Sebastian Reichel <sre@kernel.org> wrote:
> > >> > Hi,
> > >> >
> > >> > On Wed, Feb 07, 2018 at 03:58:44PM +0100, Ognjen Galic wrote:
> > >> >> 1) Charge start threshold
> > >> >> /sys/class/power_supply/BATN/charge_start_threshold
> > >> >>
> > >> >> Valid values are [0, 99]. A value of 0 turns off the
> > >> >> start threshold wear control.
> > >> >>
> > >> >> 2) Charge stop threshold
> > >> >> /sys/class/power_supply/BATN/charge_stop_threshold
> > >> >>
> > >> >> Valid values are [1, 100]. A value of 100 turns off
> > >> >> the stop threshold wear control. This must be
> > >> >> configured first.
> > >> >
> > >> > This is a new sysfs file, that should be documented.
> > >>
> > >> Right, I should have remembered about that, sorry.
> > >>
> > >> > Also this looks pretty generic. Just introduce new POWER_SUPPLY_PROP_
> > >> > entries for start/stop charging threshold and use them.
> > >>
> > >> What about doing this as a follow-up?
> > >
> > > Fine with me.
> > 
> > OK
> > 
> > Actually, I don't see any documentation whatever for ACPI battery and
> > AC power supply properties, so I guess that needs to be added in
> > general and I don't think it would be fair to ask Ognjen to do that in
> > order to get the extension in.
> > 
> > Why don't we pencil this in as work to do?
> 
> The generic ones are documented here:

Do you guys want me to send in another revision of the patch with some
documentation on the sysfs API?

> 
> Documentation/power/power_supply_class.txt
> 
> -- Sebastian
Andy Shevchenko Feb. 23, 2018, 5:21 p.m. UTC | #9
On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić <smclt30p@gmail.com> wrote:

> Do you guys want me to send in another revision of the patch with some
> documentation on the sysfs API?

I noticed I didn't apply it because of some pending changes discussed,
perhaps this one above.
So, definitely please send a new version which addresses comments.
Rafael J. Wysocki Feb. 24, 2018, 9:07 a.m. UTC | #10
On Fri, Feb 23, 2018 at 6:21 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić <smclt30p@gmail.com> wrote:
>
>> Do you guys want me to send in another revision of the patch with some
>> documentation on the sysfs API?
>
> I noticed I didn't apply it because of some pending changes discussed,
> perhaps this one above.
>
> So, definitely please send a new version which addresses comments.

No, it actually has been applied already.

What is needed is a follow-up patch, but I'll write on that later.

Thanks,
Rafael
Ognjen Galic Feb. 24, 2018, 10:31 a.m. UTC | #11
On Sat, Feb 24, 2018 at 10:07:20AM +0100, Rafael J. Wysocki wrote:
> On Fri, Feb 23, 2018 at 6:21 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić <smclt30p@gmail.com> wrote:
> >
> >> Do you guys want me to send in another revision of the patch with some
> >> documentation on the sysfs API?
> >
> > I noticed I didn't apply it because of some pending changes discussed,
> > perhaps this one above.
> >
> > So, definitely please send a new version which addresses comments.
> 
> No, it actually has been applied already.

So it has been queued up for 4.17?

If it has, that is awesome! Thanks!

> 
> What is needed is a follow-up patch, but I'll write on that later.
> 
> Thanks,
> Rafael
Ognjen Galic Feb. 24, 2018, 5:32 p.m. UTC | #12
On Sat, Feb 24, 2018 at 10:07:20AM +0100, Rafael J. Wysocki wrote:
> On Fri, Feb 23, 2018 at 6:21 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić <smclt30p@gmail.com> wrote:
> >
> >> Do you guys want me to send in another revision of the patch with some
> >> documentation on the sysfs API?
> >
> > I noticed I didn't apply it because of some pending changes discussed,
> > perhaps this one above.
> >
> > So, definitely please send a new version which addresses comments.
> 
> No, it actually has been applied already.

And also, how is it applied if it does not apply to 4.16-rc2 anymore? 
Do you need a new revision that solves the apply conflicts?
Do you need a new revision that addresses the documentation?
Did you apply it earlier than 4.16-rc2? 
When did you apply it? 
Did you apply it at all?

> 
> What is needed is a follow-up patch, but I'll write on that later.
> 
> Thanks,
> Rafael
Rafael J. Wysocki Feb. 25, 2018, 9:20 a.m. UTC | #13
On Sat, Feb 24, 2018 at 11:31 AM, Ognjen Galić <smclt30p@gmail.com> wrote:
> On Sat, Feb 24, 2018 at 10:07:20AM +0100, Rafael J. Wysocki wrote:
>> On Fri, Feb 23, 2018 at 6:21 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>> > On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić <smclt30p@gmail.com> wrote:
>> >
>> >> Do you guys want me to send in another revision of the patch with some
>> >> documentation on the sysfs API?
>> >
>> > I noticed I didn't apply it because of some pending changes discussed,
>> > perhaps this one above.
>> >
>> > So, definitely please send a new version which addresses comments.
>>
>> No, it actually has been applied already.
>
> So it has been queued up for 4.17?

Yes, it has.

> If it has, that is awesome! Thanks!

No biggie.

That said, I would like you to send a follow-up patch moving the
definitions of the charge_start_threshold and charge_stop_threshold
attributes to the power supply core as requested by Sebastian and
documenting them.

Of course, it still needs to be possible to add/remove them on demand
after registering the power supply to cover the use case at hand
AFAICS.

Thanks,
Rafael
Rafael J. Wysocki Feb. 25, 2018, 9:22 a.m. UTC | #14
On Sat, Feb 24, 2018 at 6:32 PM, Ognjen Galić <smclt30p@gmail.com> wrote:
> On Sat, Feb 24, 2018 at 10:07:20AM +0100, Rafael J. Wysocki wrote:
>> On Fri, Feb 23, 2018 at 6:21 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>> > On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić <smclt30p@gmail.com> wrote:
>> >
>> >> Do you guys want me to send in another revision of the patch with some
>> >> documentation on the sysfs API?
>> >
>> > I noticed I didn't apply it because of some pending changes discussed,
>> > perhaps this one above.
>> >
>> > So, definitely please send a new version which addresses comments.
>>
>> No, it actually has been applied already.
>
> And also, how is it applied if it does not apply to 4.16-rc2 anymore?
> Do you need a new revision that solves the apply conflicts?
> Do you need a new revision that addresses the documentation?
> Did you apply it earlier than 4.16-rc2?
> When did you apply it?
> Did you apply it at all?

Yes, I did, on top of 4.16-rc2, and I might have rebased it a bit.

Please see linux-next and I will expose the acpi-battery branch
containing this series later today and tomorrow.

>> What is needed is a follow-up patch, but I'll write on that later.

Please see my previous message in this thread regarding the above.
Andy Shevchenko Feb. 25, 2018, 12:43 p.m. UTC | #15
On Sat, Feb 24, 2018 at 11:07 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Feb 23, 2018 at 6:21 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić <smclt30p@gmail.com> wrote:
>>
>>> Do you guys want me to send in another revision of the patch with some
>>> documentation on the sysfs API?
>>
>> I noticed I didn't apply it because of some pending changes discussed,
>> perhaps this one above.
>>
>> So, definitely please send a new version which addresses comments.
>
> No, it actually has been applied already.

That's what I missed.

Thanks!
diff mbox

Patch

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 9a8f96465..0d13d30c1 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -425,6 +425,7 @@  config SURFACE3_WMI
 config THINKPAD_ACPI
 	tristate "ThinkPad ACPI Laptop Extras"
 	depends on ACPI
+	depends on ACPI_BATTERY
 	depends on INPUT
 	depends on RFKILL || RFKILL = n
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index d5eaf3b1e..1c57ee2b6 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -23,7 +23,7 @@ 
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#define TPACPI_VERSION "0.25"
+#define TPACPI_VERSION "0.26"
 #define TPACPI_SYSFS_VERSION 0x030000
 
 /*
@@ -66,6 +66,7 @@ 
 #include <linux/seq_file.h>
 #include <linux/sysfs.h>
 #include <linux/backlight.h>
+#include <linux/bitops.h>
 #include <linux/fb.h>
 #include <linux/platform_device.h>
 #include <linux/hwmon.h>
@@ -78,11 +79,13 @@ 
 #include <linux/workqueue.h>
 #include <linux/acpi.h>
 #include <linux/pci_ids.h>
+#include <linux/power_supply.h>
 #include <linux/thinkpad_acpi.h>
 #include <sound/core.h>
 #include <sound/control.h>
 #include <sound/initval.h>
 #include <linux/uaccess.h>
+#include <acpi/battery.h>
 #include <acpi/video.h>
 
 /* ThinkPad CMOS commands */
@@ -335,6 +338,7 @@  static struct {
 	u32 sensors_pdev_attrs_registered:1;
 	u32 hotkey_poll_active:1;
 	u32 has_adaptive_kbd:1;
+	u32 battery:1;
 } tp_features;
 
 static struct {
@@ -9209,6 +9213,385 @@  static struct ibm_struct mute_led_driver_data = {
 	.resume = mute_led_resume,
 };
 
+/*
+ * Battery Wear Control Driver
+ * Contact: Ognjen Galic <smclt30p@gmail.com>
+ */
+
+/* Metadata */
+
+#define GET_START	"BCTG"
+#define SET_START	"BCCS"
+#define GET_STOP	"BCSG"
+#define SET_STOP	"BCSS"
+
+#define START_ATTR "charge_start_threshold"
+#define STOP_ATTR  "charge_stop_threshold"
+
+enum {
+	BAT_ANY = 0,
+	BAT_PRIMARY = 1,
+	BAT_SECONDARY = 2
+};
+
+enum {
+	/* Error condition bit */
+	METHOD_ERR = BIT(31),
+};
+
+enum {
+	/* This is used in the get/set helpers */
+	THRESHOLD_START,
+	THRESHOLD_STOP,
+};
+
+struct tpacpi_battery_data {
+	int charge_start;
+	int start_support;
+	int charge_stop;
+	int stop_support;
+};
+
+struct tpacpi_battery_driver_data {
+	struct tpacpi_battery_data batteries[3];
+	int individual_addressing;
+};
+
+static struct tpacpi_battery_driver_data battery_info;
+
+/* ACPI helpers/functions/probes */
+
+/**
+ * This evaluates a ACPI method call specific to the battery
+ * ACPI extension. The specifics are that an error is marked
+ * in the 32rd bit of the response, so we just check that here.
+ */
+static acpi_status tpacpi_battery_acpi_eval(char *method, int *ret, int param)
+{
+	int response;
+
+	if (!acpi_evalf(hkey_handle, &response, method, "dd", param)) {
+		acpi_handle_err(hkey_handle, "%s: evaluate failed", method);
+		return AE_ERROR;
+	}
+	if (response & METHOD_ERR) {
+		acpi_handle_err(hkey_handle,
+				"%s evaluated but flagged as error", method);
+		return AE_ERROR;
+	}
+	*ret = response;
+	return AE_OK;
+}
+
+static int tpacpi_battery_get(int what, int battery, int *ret)
+{
+	switch (what) {
+	case THRESHOLD_START:
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_START, ret, battery))
+			return -ENODEV;
+
+		/* The value is in the low 8 bits of the response */
+		*ret = *ret & 0xFF;
+		return 0;
+	case THRESHOLD_STOP:
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_STOP, ret, battery))
+			return -ENODEV;
+		/* Value is in lower 8 bits */
+		*ret = *ret & 0xFF;
+		/*
+		 * On the stop value, if we return 0 that
+		 * does not make any sense. 0 means Default, which
+		 * means that charging stops at 100%, so we return
+		 * that.
+		 */
+		if (*ret == 0)
+			*ret = 100;
+		return 0;
+	default:
+		pr_crit("wrong parameter: %d", what);
+		return -EINVAL;
+	}
+}
+
+static int tpacpi_battery_set(int what, int battery, int value)
+{
+	int param, ret;
+	/* The first 8 bits are the value of the threshold */
+	param = value;
+	/* The battery ID is in bits 8-9, 2 bits */
+	param |= battery << 8;
+
+	switch (what) {
+	case THRESHOLD_START:
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(SET_START, &ret, param)) {
+			pr_err("failed to set charge threshold on battery %d",
+					battery);
+			return -ENODEV;
+		}
+		return 0;
+	case THRESHOLD_STOP:
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(SET_STOP, &ret, param)) {
+			pr_err("failed to set stop threshold: %d", battery);
+			return -ENODEV;
+		}
+		return 0;
+	default:
+		pr_crit("wrong parameter: %d", what);
+		return -EINVAL;
+	}
+}
+
+static int tpacpi_battery_probe(int battery)
+{
+	int ret = 0;
+
+	memset(&battery_info, 0, sizeof(struct tpacpi_battery_driver_data));
+	/*
+	 * 1) Get the current start threshold
+	 * 2) Check for support
+	 * 3) Get the current stop threshold
+	 * 4) Check for support
+	 */
+	if (acpi_has_method(hkey_handle, GET_START)) {
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_START, &ret, battery)) {
+			pr_err("Error probing battery %d\n", battery);
+			return -ENODEV;
+		}
+		/* Individual addressing is in bit 9 */
+		if (ret & BIT(9))
+			battery_info.individual_addressing = true;
+		/* Support is marked in bit 8 */
+		if (ret & BIT(8))
+			battery_info.batteries[battery].start_support = 1;
+		else
+			return -ENODEV;
+		if (tpacpi_battery_get(THRESHOLD_START, battery,
+			&battery_info.batteries[battery].charge_start)) {
+			pr_err("Error probing battery %d\n", battery);
+			return -ENODEV;
+		}
+	}
+	if (acpi_has_method(hkey_handle, GET_STOP)) {
+		if ACPI_FAILURE(tpacpi_battery_acpi_eval(GET_STOP, &ret, battery)) {
+			pr_err("Error probing battery stop; %d\n", battery);
+			return -ENODEV;
+		}
+		/* Support is marked in bit 8 */
+		if (ret & BIT(8))
+			battery_info.batteries[battery].stop_support = 1;
+		else
+			return -ENODEV;
+		if (tpacpi_battery_get(THRESHOLD_STOP, battery,
+			&battery_info.batteries[battery].charge_stop)) {
+			pr_err("Error probing battery stop: %d\n", battery);
+			return -ENODEV;
+		}
+	}
+	pr_info("battery %d registered (start %d, stop %d)",
+			battery,
+			battery_info.batteries[battery].charge_start,
+			battery_info.batteries[battery].charge_stop);
+
+	return 0;
+}
+
+/* General helper functions */
+
+static int tpacpi_battery_get_id(const char *battery_name)
+{
+
+	if (strcmp(battery_name, "BAT0") == 0)
+		return BAT_PRIMARY;
+	if (strcmp(battery_name, "BAT1") == 0)
+		return BAT_SECONDARY;
+	/*
+	 * If for some reason the battery is not BAT0 nor is it
+	 * BAT1, we will assume it's the default, first battery,
+	 * AKA primary.
+	 */
+	pr_warn("unknown battery %s, assuming primary", battery_name);
+	return BAT_PRIMARY;
+}
+
+/* sysfs interface */
+
+static ssize_t tpacpi_battery_store(int what,
+				    struct device *dev,
+				    const char *buf, size_t count)
+{
+	struct power_supply *supply = to_power_supply(dev);
+	unsigned long value;
+	int battery, rval;
+	/*
+	 * Some systems have support for more than
+	 * one battery. If that is the case,
+	 * tpacpi_battery_probe marked that addressing
+	 * them individually is supported, so we do that
+	 * based on the device struct.
+	 *
+	 * On systems that are not supported, we assume
+	 * the primary as most of the ACPI calls fail
+	 * with "Any Battery" as the parameter.
+	 */
+	if (battery_info.individual_addressing)
+		/* BAT_PRIMARY or BAT_SECONDARY */
+		battery = tpacpi_battery_get_id(supply->desc->name);
+	else
+		battery = BAT_PRIMARY;
+
+	rval = kstrtoul(buf, 10, &value);
+	if (rval)
+		return rval;
+
+	switch (what) {
+	case THRESHOLD_START:
+		if (!battery_info.batteries[battery].start_support)
+			return -ENODEV;
+		/* valid values are [0, 99] */
+		if (value < 0 || value > 99)
+			return -EINVAL;
+		if (value > battery_info.batteries[battery].charge_stop)
+			return -EINVAL;
+		if (tpacpi_battery_set(THRESHOLD_START, battery, value))
+			return -ENODEV;
+		battery_info.batteries[battery].charge_start = value;
+		return count;
+
+	case THRESHOLD_STOP:
+		if (!battery_info.batteries[battery].stop_support)
+			return -ENODEV;
+		/* valid values are [1, 100] */
+		if (value < 1 || value > 100)
+			return -EINVAL;
+		if (value < battery_info.batteries[battery].charge_start)
+			return -EINVAL;
+		battery_info.batteries[battery].charge_stop = value;
+		/*
+		 * When 100 is passed to stop, we need to flip
+		 * it to 0 as that the EC understands that as
+		 * "Default", which will charge to 100%
+		 */
+		if (value == 100)
+			value = 0;
+		if (tpacpi_battery_set(THRESHOLD_STOP, battery, value))
+			return -EINVAL;
+		return count;
+	default:
+		pr_crit("Wrong parameter: %d", what);
+		return -EINVAL;
+	}
+	return count;
+}
+
+static ssize_t tpacpi_battery_show(int what,
+				   struct device *dev,
+				   char *buf)
+{
+	struct power_supply *supply = to_power_supply(dev);
+	int ret, battery;
+	/*
+	 * Some systems have support for more than
+	 * one battery. If that is the case,
+	 * tpacpi_battery_probe marked that addressing
+	 * them individually is supported, so we;
+	 * based on the device struct.
+	 *
+	 * On systems that are not supported, we assume
+	 * the primary as most of the ACPI calls fail
+	 * with "Any Battery" as the parameter.
+	 */
+	if (battery_info.individual_addressing)
+		/* BAT_PRIMARY or BAT_SECONDARY */
+		battery = tpacpi_battery_get_id(supply->desc->name);
+	else
+		battery = BAT_PRIMARY;
+	if (tpacpi_battery_get(what, battery, &ret))
+		return -ENODEV;
+	return sprintf(buf, "%d\n", ret);
+}
+
+static ssize_t charge_start_threshold_show(struct device *device,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return tpacpi_battery_show(THRESHOLD_START, device, buf);
+}
+
+static ssize_t charge_stop_threshold_show(struct device *device,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return tpacpi_battery_show(THRESHOLD_STOP, device, buf);
+}
+
+static ssize_t charge_start_threshold_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	return tpacpi_battery_store(THRESHOLD_START, dev, buf, count);
+}
+
+static ssize_t charge_stop_threshold_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	return tpacpi_battery_store(THRESHOLD_STOP, dev, buf, count);
+}
+
+static DEVICE_ATTR_RW(charge_start_threshold);
+static DEVICE_ATTR_RW(charge_stop_threshold);
+
+static struct attribute *tpacpi_battery_attrs[] = {
+	&dev_attr_charge_start_threshold.attr,
+	&dev_attr_charge_stop_threshold.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(tpacpi_battery);
+
+/* ACPI battery hooking */
+
+static int tpacpi_battery_add(struct power_supply *battery)
+{
+	int batteryid = tpacpi_battery_get_id(battery->desc->name);
+
+	if (tpacpi_battery_probe(batteryid))
+		return -ENODEV;
+	if (device_add_groups(&battery->dev, tpacpi_battery_groups))
+		return -ENODEV;
+	return 0;
+}
+
+static int tpacpi_battery_remove(struct power_supply *battery)
+{
+	device_remove_groups(&battery->dev, tpacpi_battery_groups);
+	return 0;
+}
+
+static struct acpi_battery_hook battery_hook = {
+	.add_battery = tpacpi_battery_add,
+	.remove_battery = tpacpi_battery_remove,
+	.name = "ThinkPad Battery Extension",
+};
+
+/* Subdriver init/exit */
+
+static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
+{
+	battery_hook_register(&battery_hook);
+	return 0;
+}
+
+static void tpacpi_battery_exit(void)
+{
+	battery_hook_unregister(&battery_hook);
+}
+
+static struct ibm_struct battery_driver_data = {
+	.name = "battery",
+	.exit = tpacpi_battery_exit,
+};
+
 /****************************************************************************
  ****************************************************************************
  *
@@ -9655,6 +10038,10 @@  static struct ibm_init_struct ibms_init[] __initdata = {
 		.init = mute_led_init,
 		.data = &mute_led_driver_data,
 	},
+	{
+		.init = tpacpi_battery_init,
+		.data = &battery_driver_data,
+	},
 };
 
 static int __init set_ibm_param(const char *val, const struct kernel_param *kp)