| Message ID | 20220828192920.805253-3-lkml@vorpal.se (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: Battery charge mode in toshiba_acpi | expand |
Hi Arvid,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on 1c23f9e627a7b412978b4e852793c5e3c3efc555]
url: https://github.com/intel-lab-lkp/linux/commits/Arvid-Norlander/platform-x86-Battery-charge-mode-in-toshiba_acpi/20220829-033110
base: 1c23f9e627a7b412978b4e852793c5e3c3efc555
config: x86_64-randconfig-a014-20220829 (https://download.01.org/0day-ci/archive/20220829/202208290605.gE9IGbxE-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/a85fa4a6b19ae082f6018a07da93db965fb5ba11
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Arvid-Norlander/platform-x86-Battery-charge-mode-in-toshiba_acpi/20220829-033110
git checkout a85fa4a6b19ae082f6018a07da93db965fb5ba11
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/platform/x86/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/platform/x86/toshiba_acpi.c:2992:6: warning: variable 'status' set but not used [-Wunused-but-set-variable]
int status;
^
1 warning generated.
vim +/status +2992 drivers/platform/x86/toshiba_acpi.c
2984
2985
2986 /* ACPI battery hooking */
2987 static ssize_t charge_control_end_threshold_show(struct device *device,
2988 struct device_attribute *attr,
2989 char *buf)
2990 {
2991 int state;
> 2992 int status;
2993
2994 if (toshiba_acpi == NULL) {
2995 pr_err("Toshiba ACPI object invalid\n");
2996 return -ENODEV;
2997 }
2998
2999 status = toshiba_battery_charge_mode_get(toshiba_acpi, &state);
3000 if (state == 1)
3001 return sprintf(buf, "80\n");
3002 else
3003 return sprintf(buf, "100\n");
3004 }
3005
Hi Arvid,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on 1c23f9e627a7b412978b4e852793c5e3c3efc555]
url: https://github.com/intel-lab-lkp/linux/commits/Arvid-Norlander/platform-x86-Battery-charge-mode-in-toshiba_acpi/20220829-033110
base: 1c23f9e627a7b412978b4e852793c5e3c3efc555
config: x86_64-randconfig-a002-20220829 (https://download.01.org/0day-ci/archive/20220829/202208290628.Er7oPvnk-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/a85fa4a6b19ae082f6018a07da93db965fb5ba11
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Arvid-Norlander/platform-x86-Battery-charge-mode-in-toshiba_acpi/20220829-033110
git checkout a85fa4a6b19ae082f6018a07da93db965fb5ba11
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/platform/x86/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/platform/x86/toshiba_acpi.c: In function 'charge_control_end_threshold_show':
>> drivers/platform/x86/toshiba_acpi.c:2992:13: warning: variable 'status' set but not used [-Wunused-but-set-variable]
2992 | int status;
| ^~~~~~
vim +/status +2992 drivers/platform/x86/toshiba_acpi.c
2984
2985
2986 /* ACPI battery hooking */
2987 static ssize_t charge_control_end_threshold_show(struct device *device,
2988 struct device_attribute *attr,
2989 char *buf)
2990 {
2991 int state;
> 2992 int status;
2993
2994 if (toshiba_acpi == NULL) {
2995 pr_err("Toshiba ACPI object invalid\n");
2996 return -ENODEV;
2997 }
2998
2999 status = toshiba_battery_charge_mode_get(toshiba_acpi, &state);
3000 if (state == 1)
3001 return sprintf(buf, "80\n");
3002 else
3003 return sprintf(buf, "100\n");
3004 }
3005
Hi, On 2022-08-29 00:15, kernel test robot wrote: > Hi Arvid, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on 1c23f9e627a7b412978b4e852793c5e3c3efc555] <snip> > vim +/status +2992 drivers/platform/x86/toshiba_acpi.c > > 2984 > 2985 > 2986 /* ACPI battery hooking */ > 2987 static ssize_t charge_control_end_threshold_show(struct device *device, > 2988 struct device_attribute *attr, > 2989 char *buf) > 2990 { > 2991 int state; >> 2992 int status; Will be fixed in the next version, it should be used. Waiting for human feedback too first. It would be nice to see these warnings locally, anyone know how to turn them on? > 2993 > 2994 if (toshiba_acpi == NULL) { > 2995 pr_err("Toshiba ACPI object invalid\n"); > 2996 return -ENODEV; > 2997 } > 2998 > 2999 status = toshiba_battery_charge_mode_get(toshiba_acpi, &state); > 3000 if (state == 1) > 3001 return sprintf(buf, "80\n"); > 3002 else > 3003 return sprintf(buf, "100\n"); > 3004 } > 3005 > Best regards, Arvid Norlander
On Mon, Aug 29, 2022 at 12:50 AM Arvid Norlander <lkml@vorpal.se> wrote: > > It would be nice to see these warnings locally, anyone know how to turn them on? Compiling with W=1 (and possibly with Clang) should show them. Or doesn't that work for you? Cheers, Miguel
On Sun, Aug 28, 2022 at 3:16 PM kernel test robot <lkp@intel.com> wrote: > > Hi Arvid, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on 1c23f9e627a7b412978b4e852793c5e3c3efc555] > > url: https://github.com/intel-lab-lkp/linux/commits/Arvid-Norlander/platform-x86-Battery-charge-mode-in-toshiba_acpi/20220829-033110 > base: 1c23f9e627a7b412978b4e852793c5e3c3efc555 > config: x86_64-randconfig-a014-20220829 (https://download.01.org/0day-ci/archive/20220829/202208290605.gE9IGbxE-lkp@intel.com/config) Note: this was from a randconfig; it's highly likely you need this funky config to repro. ^ should be the link to the config.
diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c index c927d5d0f8cd..8e272b4336c6 100644 --- a/drivers/platform/x86/toshiba_acpi.c +++ b/drivers/platform/x86/toshiba_acpi.c @@ -44,6 +44,7 @@ #include <linux/rfkill.h> #include <linux/iio/iio.h> #include <linux/toshiba.h> +#include <acpi/battery.h> #include <acpi/video.h> MODULE_AUTHOR("John Belmonte"); @@ -2981,6 +2982,88 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev) return 0; } + +/* ACPI battery hooking */ +static ssize_t charge_control_end_threshold_show(struct device *device, + struct device_attribute *attr, + char *buf) +{ + int state; + int status; + + if (toshiba_acpi == NULL) { + pr_err("Toshiba ACPI object invalid\n"); + return -ENODEV; + } + + status = toshiba_battery_charge_mode_get(toshiba_acpi, &state); + if (state == 1) + return sprintf(buf, "80\n"); + else + return sprintf(buf, "100\n"); +} + +static ssize_t charge_control_end_threshold_store(struct device *dev, + struct device_attribute *attr, + const char *buf, + size_t count) +{ + u32 value; + int rval; + + if (toshiba_acpi == NULL) { + pr_err("Toshiba ACPI object invalid\n"); + return -ENODEV; + } + + rval = kstrtou32(buf, 10, &value); + if (rval) + return rval; + + if (value < 1 || value > 100) + return -EINVAL; + rval = toshiba_battery_charge_mode_set(toshiba_acpi, + (value < 90) ? 1 : 0); + if (rval < 0) + return rval; + else + return count; +} + +static DEVICE_ATTR_RW(charge_control_end_threshold); + +static struct attribute *toshiba_acpi_battery_attrs[] = { + &dev_attr_charge_control_end_threshold.attr, + NULL, +}; + +ATTRIBUTE_GROUPS(toshiba_acpi_battery); + +static int toshiba_acpi_battery_add(struct power_supply *battery) +{ + if (toshiba_acpi == NULL) { + pr_err("Init order issue\n"); + return -ENODEV; + } + if (!toshiba_acpi->battery_charge_mode_supported) + return -ENODEV; + if (device_add_groups(&battery->dev, toshiba_acpi_battery_groups)) + return -ENODEV; + return 0; +} + +static int toshiba_acpi_battery_remove(struct power_supply *battery) +{ + device_remove_groups(&battery->dev, toshiba_acpi_battery_groups); + return 0; +} + +static struct acpi_battery_hook battery_hook = { + .add_battery = toshiba_acpi_battery_add, + .remove_battery = toshiba_acpi_battery_remove, + .name = "Toshiba Battery Extension", +}; + static void print_supported_features(struct toshiba_acpi_dev *dev) { pr_info("Supported laptop features:"); @@ -3063,6 +3146,9 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev) rfkill_destroy(dev->wwan_rfk); } + if (dev->battery_charge_mode_supported) + battery_hook_unregister(&battery_hook); + if (toshiba_acpi) toshiba_acpi = NULL; @@ -3246,6 +3332,13 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev) toshiba_acpi = dev; + /* + * As the battery hook relies on the static variable toshiba_acpi being + * set, this must be done after toshiba_acpi is assigned. + */ + if (dev->battery_charge_mode_supported) + battery_hook_register(&battery_hook); + return 0; error:
This commit adds the ACPI battery hook which in turns adds the sysfs entries. Because the Toshiba laptops only support two modes (eco or normal), which in testing correspond to 80% and 100% we simply round to the nearest possible level when set. It is possible that Toshiba laptops other than the Z830 has different set points for the charging. If so, a quirk table could be introduced in the future for this. For now, assume that all laptops that support this feature work the same way. Tested on a Toshiba Satellite Z830. Signed-off-by: Arvid Norlander <lkml@vorpal.se> --- drivers/platform/x86/toshiba_acpi.c | 93 +++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+)