| Message ID | 20210111162237.3469-1-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v9] platform/x86: thinkpad_acpi: Add platform profile support | expand |
Hi Mark,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.11-rc3 next-20210115]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Mark-Pearson/platform-x86-thinkpad_acpi-Add-platform-profile-support/20210112-003248
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 7c53f6b671f4aba70ff15e1b05148b10d58c2837
config: x86_64-rhel (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/958de6527b22cc8744001da18b8e9a9c0b85c201
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mark-Pearson/platform-x86-thinkpad_acpi-Add-platform-profile-support/20210112-003248
git checkout 958de6527b22cc8744001da18b8e9a9c0b85c201
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/platform/x86/thinkpad_acpi.c:69:10: fatal error: linux/platform_profile.h: No such file or directory
69 | #include <linux/platform_profile.h>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
vim +69 drivers/platform/x86/thinkpad_acpi.c
13
14 /*
15 * Changelog:
16 * 2007-10-20 changelog trimmed down
17 *
18 * 2007-03-27 0.14 renamed to thinkpad_acpi and moved to
19 * drivers/misc.
20 *
21 * 2006-11-22 0.13 new maintainer
22 * changelog now lives in git commit history, and will
23 * not be updated further in-file.
24 *
25 * 2005-03-17 0.11 support for 600e, 770x
26 * thanks to Jamie Lentin <lentinj@dial.pipex.com>
27 *
28 * 2005-01-16 0.9 use MODULE_VERSION
29 * thanks to Henrik Brix Andersen <brix@gentoo.org>
30 * fix parameter passing on module loading
31 * thanks to Rusty Russell <rusty@rustcorp.com.au>
32 * thanks to Jim Radford <radford@blackbean.org>
33 * 2004-11-08 0.8 fix init error case, don't return from a macro
34 * thanks to Chris Wright <chrisw@osdl.org>
35 */
36
37 #include <linux/kernel.h>
38 #include <linux/module.h>
39 #include <linux/init.h>
40 #include <linux/types.h>
41 #include <linux/string.h>
42 #include <linux/list.h>
43 #include <linux/mutex.h>
44 #include <linux/sched.h>
45 #include <linux/sched/signal.h>
46 #include <linux/kthread.h>
47 #include <linux/freezer.h>
48 #include <linux/delay.h>
49 #include <linux/slab.h>
50 #include <linux/nvram.h>
51 #include <linux/proc_fs.h>
52 #include <linux/seq_file.h>
53 #include <linux/sysfs.h>
54 #include <linux/backlight.h>
55 #include <linux/bitops.h>
56 #include <linux/fb.h>
57 #include <linux/platform_device.h>
58 #include <linux/hwmon.h>
59 #include <linux/hwmon-sysfs.h>
60 #include <linux/input.h>
61 #include <linux/leds.h>
62 #include <linux/rfkill.h>
63 #include <linux/dmi.h>
64 #include <linux/jiffies.h>
65 #include <linux/workqueue.h>
66 #include <linux/acpi.h>
67 #include <linux/pci.h>
68 #include <linux/power_supply.h>
> 69 #include <linux/platform_profile.h>
70 #include <sound/core.h>
71 #include <sound/control.h>
72 #include <sound/initval.h>
73 #include <linux/uaccess.h>
74 #include <acpi/battery.h>
75 #include <acpi/video.h>
76
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi, On 1/11/21 5:22 PM, Mark Pearson wrote: > Add support to thinkpad_acpi for Lenovo platforms that have DYTC > version 5 support or newer to use the platform profile feature. > > This will allow users to determine and control the platform modes > between low-power, balanced operation and performance modes. > > Signed-off-by: Mark Pearson <markpearson@lenovo.com> Now that the acpi/platform_profile changes have landed I have merged this patch (solving a trivial conflict caused by the keyboard_lang changes). Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > Changes in v2: > Address (hopefully) all recommendations from review including: > - use IS_ENABLED instead of IS_DEFINED > - update driver to work with all the fixes in platform_profile update > - improve error handling for invalid inputs > - move tracking of current profile mode into this driver > > Changes in v3: > - version update for patch series > > Changes in v4: > - Rebase on top of palm sensor patch which led to a little bit of file > restructuring/clean up > - Use BIT macro where applicable > - Formatting fixes > - Check sysfs node created on exit function > - implement and use DYTC_SET_COMMAND macro > - in case of failure setting performance mode make sure CQL mode is > enabled again before returning. > - Clean up initialisation and error handling code > > Changes in v5: > - Use atomic_int with ignoring events > - Add mutex when accessing ACPI information > - Clean up registration code > - Use cached value in profile_get function > - Add dytc_cql_command function to clean up and provide common handler > - Update to work with changes in platform_profile implementation > > Changes in v6: > - Update file to build against update in v6 platform_profile > > Changes in v7 & v8: > - version bump along with rest of patch series > > Changes in v9: > - Update define names to be correct with the linux-pm branches accepted > version > - Rename DYTC_MODE_QUIET to LOW_POWER just because it's confusing. It > was originally based on internal documentation. > Note - originally this patch was part of a series but with the other two > pieces being accepted into the acpi maintainers branch this one becomes > standalone. > > drivers/platform/x86/thinkpad_acpi.c | 294 ++++++++++++++++++++++++++- > 1 file changed, 288 insertions(+), 6 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index e03df2881dc6..c5645331cfac 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -66,6 +66,7 @@ > #include <linux/acpi.h> > #include <linux/pci.h> > #include <linux/power_supply.h> > +#include <linux/platform_profile.h> > #include <sound/core.h> > #include <sound/control.h> > #include <sound/initval.h> > @@ -9854,16 +9855,27 @@ static bool has_lapsensor; > static bool palm_state; > static bool lap_state; > > -static int lapsensor_get(bool *present, bool *state) > +static int dytc_command(int command, int *output) > { > acpi_handle dytc_handle; > - int output; > > - *present = false; > - if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) > + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) { > + /* Platform doesn't support DYTC */ > return -ENODEV; > - if (!acpi_evalf(dytc_handle, &output, NULL, "dd", DYTC_CMD_GET)) > + } > + if (!acpi_evalf(dytc_handle, output, NULL, "dd", command)) > return -EIO; > + return 0; > +} > + > +static int lapsensor_get(bool *present, bool *state) > +{ > + int output, err; > + > + *present = false; > + err = dytc_command(DYTC_CMD_GET, &output); > + if (err) > + return err; > > *present = true; /*If we get his far, we have lapmode support*/ > *state = output & BIT(DYTC_GET_LAPMODE_BIT) ? true : false; > @@ -9982,6 +9994,264 @@ static struct ibm_struct proxsensor_driver_data = { > .exit = proxsensor_exit, > }; > > +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) > + > +/************************************************************************* > + * DYTC Platform Profile interface > + */ > + > +#define DYTC_CMD_QUERY 0 /* To get DYTC status - enable/revision */ > +#define DYTC_CMD_SET 1 /* To enable/disable IC function mode */ > +#define DYTC_CMD_RESET 0x1ff /* To reset back to default */ > + > +#define DYTC_QUERY_ENABLE_BIT 8 /* Bit 8 - 0 = disabled, 1 = enabled */ > +#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */ > +#define DYTC_QUERY_REV_BIT 28 /* Bits 28 - 31 - revision */ > + > +#define DYTC_GET_FUNCTION_BIT 8 /* Bits 8-11 - function setting */ > +#define DYTC_GET_MODE_BIT 12 /* Bits 12-15 - mode setting */ > + > +#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - function setting */ > +#define DYTC_SET_MODE_BIT 16 /* Bits 16-19 - mode setting */ > +#define DYTC_SET_VALID_BIT 20 /* Bit 20 - 1 = on, 0 = off */ > + > +#define DYTC_FUNCTION_STD 0 /* Function = 0, standard mode */ > +#define DYTC_FUNCTION_CQL 1 /* Function = 1, lap mode */ > +#define DYTC_FUNCTION_MMC 11 /* Function = 11, desk mode */ > + > +#define DYTC_MODE_PERFORM 2 /* High power mode aka performance */ > +#define DYTC_MODE_LOWPOWER 3 /* Low power mode */ > +#define DYTC_MODE_BALANCE 0xF /* Default mode aka balanced */ > + > +#define DYTC_SET_COMMAND(function, mode, on) \ > + (DYTC_CMD_SET | (function) << DYTC_SET_FUNCTION_BIT | \ > + (mode) << DYTC_SET_MODE_BIT | \ > + (on) << DYTC_SET_VALID_BIT) > + > +#define DYTC_DISABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 0) > + > +#define DYTC_ENABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 1) > + > +static bool dytc_profile_available; > +static enum platform_profile_option dytc_current_profile; > +static atomic_t dytc_ignore_event = ATOMIC_INIT(0); > +static DEFINE_MUTEX(dytc_mutex); > + > +static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile) > +{ > + switch (dytcmode) { > + case DYTC_MODE_LOWPOWER: > + *profile = PLATFORM_PROFILE_LOW_POWER; > + break; > + case DYTC_MODE_BALANCE: > + *profile = PLATFORM_PROFILE_BALANCED; > + break; > + case DYTC_MODE_PERFORM: > + *profile = PLATFORM_PROFILE_PERFORMANCE; > + break; > + default: /* Unknown mode */ > + return -EINVAL; > + } > + return 0; > +} > + > +static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode) > +{ > + switch (profile) { > + case PLATFORM_PROFILE_LOW_POWER: > + *perfmode = DYTC_MODE_LOWPOWER; > + break; > + case PLATFORM_PROFILE_BALANCED: > + *perfmode = DYTC_MODE_BALANCE; > + break; > + case PLATFORM_PROFILE_PERFORMANCE: > + *perfmode = DYTC_MODE_PERFORM; > + break; > + default: /* Unknown profile */ > + return -EOPNOTSUPP; > + } > + return 0; > +} > + > +/* > + * dytc_profile_get: Function to register with platform_profile > + * handler. Returns current platform profile. > + */ > +int dytc_profile_get(struct platform_profile_handler *pprof, > + enum platform_profile_option *profile) > +{ > + *profile = dytc_current_profile; > + return 0; > +} > + > +/* > + * Helper function - check if we are in CQL mode and if we are > + * - disable CQL, > + * - run the command > + * - enable CQL > + * If not in CQL mode, just run the command > + */ > +int dytc_cql_command(int command, int *output) > +{ > + int err, cmd_err, dummy; > + int cur_funcmode; > + > + /* Determine if we are in CQL mode. This alters the commands we do */ > + err = dytc_command(DYTC_CMD_GET, output); > + if (err) > + return err; > + > + cur_funcmode = (*output >> DYTC_GET_FUNCTION_BIT) & 0xF; > + /* Check if we're OK to return immediately */ > + if ((command == DYTC_CMD_GET) && (cur_funcmode != DYTC_FUNCTION_CQL)) > + return 0; > + > + if (cur_funcmode == DYTC_FUNCTION_CQL) { > + atomic_inc(&dytc_ignore_event); > + err = dytc_command(DYTC_DISABLE_CQL, &dummy); > + if (err) > + return err; > + } > + > + cmd_err = dytc_command(command, output); > + /* Check return condition after we've restored CQL state */ > + > + if (cur_funcmode == DYTC_FUNCTION_CQL) { > + err = dytc_command(DYTC_ENABLE_CQL, &dummy); > + if (err) > + return err; > + } > + > + return cmd_err; > +} > + > +/* > + * dytc_profile_set: Function to register with platform_profile > + * handler. Sets current platform profile. > + */ > +int dytc_profile_set(struct platform_profile_handler *pprof, > + enum platform_profile_option profile) > +{ > + int output; > + int err; > + > + if (!dytc_profile_available) > + return -ENODEV; > + > + err = mutex_lock_interruptible(&dytc_mutex); > + if (err) > + return err; > + > + if (profile == PLATFORM_PROFILE_BALANCED) { > + /* To get back to balanced mode we just issue a reset command */ > + err = dytc_command(DYTC_CMD_RESET, &output); > + if (err) > + goto unlock; > + } else { > + int perfmode; > + > + err = convert_profile_to_dytc(profile, &perfmode); > + if (err) > + goto unlock; > + > + /* Determine if we are in CQL mode. This alters the commands we do */ > + err = dytc_cql_command(DYTC_SET_COMMAND(DYTC_FUNCTION_MMC, perfmode, 1), &output); > + if (err) > + goto unlock; > + } > + /* Success - update current profile */ > + dytc_current_profile = profile; > +unlock: > + mutex_unlock(&dytc_mutex); > + return err; > +} > + > +static void dytc_profile_refresh(void) > +{ > + enum platform_profile_option profile; > + int output, err; > + int perfmode; > + > + mutex_lock(&dytc_mutex); > + err = dytc_cql_command(DYTC_CMD_GET, &output); > + mutex_unlock(&dytc_mutex); > + if (err) > + return; > + > + perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF; > + convert_dytc_to_profile(perfmode, &profile); > + if (profile != dytc_current_profile) { > + dytc_current_profile = profile; > + platform_profile_notify(); > + } > +} > + > +static struct platform_profile_handler dytc_profile = { > + .profile_get = dytc_profile_get, > + .profile_set = dytc_profile_set, > +}; > + > +static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) > +{ > + int err, output; > + > + /* Setup supported modes */ > + set_bit(PLATFORM_PROFILE_LOW_POWER, dytc_profile.choices); > + set_bit(PLATFORM_PROFILE_BALANCED, dytc_profile.choices); > + set_bit(PLATFORM_PROFILE_PERFORMANCE, dytc_profile.choices); > + > + dytc_profile_available = false; > + err = dytc_command(DYTC_CMD_QUERY, &output); > + /* > + * If support isn't available (ENODEV) then don't return an error > + * and don't create the sysfs group > + */ > + if (err == -ENODEV) > + return 0; > + /* For all other errors we can flag the failure */ > + if (err) > + return err; > + > + /* Check DYTC is enabled and supports mode setting */ > + if (output & BIT(DYTC_QUERY_ENABLE_BIT)) { > + /* Only DYTC v5.0 and later has this feature. */ > + int dytc_version; > + > + dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF; > + if (dytc_version >= 5) { > + dbg_printk(TPACPI_DBG_INIT, > + "DYTC version %d: thermal mode available\n", dytc_version); > + /* Create platform_profile structure and register */ > + err = platform_profile_register(&dytc_profile); > + /* > + * If for some reason platform_profiles aren't enabled > + * don't quit terminally. > + */ > + if (err) > + return 0; > + > + dytc_profile_available = true; > + /* Ensure initial values are correct */ > + dytc_profile_refresh(); > + } > + } > + return 0; > +} > + > +static void dytc_profile_exit(void) > +{ > + if (dytc_profile_available) { > + dytc_profile_available = false; > + platform_profile_remove(); > + } > +} > + > +static struct ibm_struct dytc_profile_driver_data = { > + .name = "dytc-profile", > + .exit = dytc_profile_exit, > +}; > +#endif /* CONFIG_ACPI_PLATFORM_PROFILE */ > + > /**************************************************************************** > **************************************************************************** > * > @@ -10030,8 +10300,14 @@ static void tpacpi_driver_event(const unsigned int hkey_event) > mutex_unlock(&kbdlight_mutex); > } > > - if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) > + if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) { > lapsensor_refresh(); > +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) > + /* If we are already accessing DYTC then skip dytc update */ > + if (!atomic_add_unless(&dytc_ignore_event, -1, 0)) > + dytc_profile_refresh(); > +#endif > + } > } > > static void hotkey_driver_event(const unsigned int scancode) > @@ -10474,6 +10750,12 @@ static struct ibm_init_struct ibms_init[] __initdata = { > .init = tpacpi_proxsensor_init, > .data = &proxsensor_driver_data, > }, > +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) > + { > + .init = tpacpi_dytc_profile_init, > + .data = &dytc_profile_driver_data, > + }, > +#endif > }; > > static int __init set_ibm_param(const char *val, const struct kernel_param *kp) >
On 02/02/2021 09:49, Hans de Goede wrote: > Hi, > > On 1/11/21 5:22 PM, Mark Pearson wrote: >> Add support to thinkpad_acpi for Lenovo platforms that have DYTC >> version 5 support or newer to use the platform profile feature. >> >> This will allow users to determine and control the platform modes >> between low-power, balanced operation and performance modes. >> >> Signed-off-by: Mark Pearson <markpearson@lenovo.com> > > Now that the acpi/platform_profile changes have landed I have > merged this patch (solving a trivial conflict caused by the > keyboard_lang changes). > > Thank you for your patch, I've applied this patch to my review-hans > branch: > https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans > > Note it will show up in my review-hans branch once I've pushed my > local branch there, which might take a while. > > Once I've run some tests on this branch the patches there will be > added to the platform-drivers-x86/for-next branch and eventually > will be included in the pdx86 pull-request to Linus for the next > merge-window. > Thanks Hans - sounds great. Let me know if you find any issues or need any extra tests running. Happy to do that Mark
Hi, On 2/3/21 3:46 PM, Mark Pearson wrote: > On 02/02/2021 09:49, Hans de Goede wrote: >> Hi, >> >> On 1/11/21 5:22 PM, Mark Pearson wrote: >>> Add support to thinkpad_acpi for Lenovo platforms that have DYTC >>> version 5 support or newer to use the platform profile feature. >>> >>> This will allow users to determine and control the platform modes >>> between low-power, balanced operation and performance modes. >>> >>> Signed-off-by: Mark Pearson <markpearson@lenovo.com> >> >> Now that the acpi/platform_profile changes have landed I have >> merged this patch (solving a trivial conflict caused by the >> keyboard_lang changes). >> >> Thank you for your patch, I've applied this patch to my review-hans >> branch: >> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans >> >> Note it will show up in my review-hans branch once I've pushed my >> local branch there, which might take a while. >> >> Once I've run some tests on this branch the patches there will be >> added to the platform-drivers-x86/for-next branch and eventually >> will be included in the pdx86 pull-request to Linus for the next >> merge-window. >> > Thanks Hans - sounds great. > Let me know if you find any issues or need any extra tests running. So the build-bots have found 2 issues: 1. Some symbols which are not exported need to be marked static (I will fix this myself, that is the easiest / fastest): drivers/platform/x86/thinkpad_acpi.c:10081:5: warning: no previous prototype for 'dytc_profile_get' [-Wmissing-prototypes] drivers/platform/x86/thinkpad_acpi.c:10095:5: warning: no previous prototype for 'dytc_cql_command' [-Wmissing-prototypes] drivers/platform/x86/thinkpad_acpi.c:10133:5: warning: no previous prototype for 'dytc_profile_set' [-Wmissing-prototypes] 2. This is a bigger problem, this is the result of a random-config test-build and I'm pretty sure that the issue is that acpi_platform was build as a module while thinkpad_acpi was builtin and builtin code cannot rely on modules. drivers/platform/x86/thinkpad_acpi.c:10186: undefined reference to `platform_profile_notify' drivers/platform/x86/thinkpad_acpi.c:10226: undefined reference to `platform_profile_register' drivers/platform/x86/thinkpad_acpi.c:10246: undefined reference to `platform_profile_remove' There are 2 ways to solve this: 1. Change #if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) to: #if IS_REACHABLE(CONFIG_ACPI_PLATFORM_PROFILE) Which will disable the platform-profile support in acpi_thinkpad in the above scenario. or. 2. Drop the #if IS_ENABLED(...) and add a depends on ACPI_PLATFORM_PROFILE To the THINKPAD_ACPI Kconfig symbol. I personally think 2. would be slightly better as it ensures that platform-profile support is always available when thinkpad_acpi is build, hopefully leading to less confusing bug-reports about it sometimes not working. If you can let me know what you want, then I can fix this locally too and get the fix in place before the merge-window opens. Regards, Hans
Hi Hans On 04/02/2021 04:18, Hans de Goede wrote: > Hi, > > On 2/3/21 3:46 PM, Mark Pearson wrote: >> On 02/02/2021 09:49, Hans de Goede wrote: >>> Hi, >>> >>> On 1/11/21 5:22 PM, Mark Pearson wrote: >>>> Add support to thinkpad_acpi for Lenovo platforms that have DYTC >>>> version 5 support or newer to use the platform profile feature. >>>> >>>> This will allow users to determine and control the platform modes >>>> between low-power, balanced operation and performance modes. >>>> >>>> Signed-off-by: Mark Pearson <markpearson@lenovo.com> >>> >>> Now that the acpi/platform_profile changes have landed I have >>> merged this patch (solving a trivial conflict caused by the >>> keyboard_lang changes). >>> >>> Thank you for your patch, I've applied this patch to my review-hans >>> branch: >>> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans >>> >>> Note it will show up in my review-hans branch once I've pushed my >>> local branch there, which might take a while. >>> >>> Once I've run some tests on this branch the patches there will be >>> added to the platform-drivers-x86/for-next branch and eventually >>> will be included in the pdx86 pull-request to Linus for the next >>> merge-window. >>> >> Thanks Hans - sounds great. >> Let me know if you find any issues or need any extra tests running. > > So the build-bots have found 2 issues: > > 1. Some symbols which are not exported need to be marked static (I will fix this myself, > that is the easiest / fastest): > > drivers/platform/x86/thinkpad_acpi.c:10081:5: warning: no previous prototype for 'dytc_profile_get' [-Wmissing-prototypes] > drivers/platform/x86/thinkpad_acpi.c:10095:5: warning: no previous prototype for 'dytc_cql_command' [-Wmissing-prototypes] > drivers/platform/x86/thinkpad_acpi.c:10133:5: warning: no previous prototype for 'dytc_profile_set' [-Wmissing-prototypes] > > 2. This is a bigger problem, this is the result of a random-config test-build and I'm > pretty sure that the issue is that acpi_platform was build as a module while > thinkpad_acpi was builtin and builtin code cannot rely on modules. > > drivers/platform/x86/thinkpad_acpi.c:10186: undefined reference to `platform_profile_notify' > drivers/platform/x86/thinkpad_acpi.c:10226: undefined reference to `platform_profile_register' > drivers/platform/x86/thinkpad_acpi.c:10246: undefined reference to `platform_profile_remove' > > There are 2 ways to solve this: > > 1. Change > > #if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) > > to: > > #if IS_REACHABLE(CONFIG_ACPI_PLATFORM_PROFILE) > > Which will disable the platform-profile support in acpi_thinkpad in the above scenario. > > or. > > 2. Drop the #if IS_ENABLED(...) and add a > > depends on ACPI_PLATFORM_PROFILE > > To the THINKPAD_ACPI Kconfig symbol. > > > I personally think 2. would be slightly better as it ensures that platform-profile > support is always available when thinkpad_acpi is build, hopefully leading to less > confusing bug-reports about it sometimes not working. > > If you can let me know what you want, then I can fix this locally too and get > the fix in place before the merge-window opens. > > Regards, > > Hans > I think you've fixed both of those already - and I agree with #2. Thanks for jumping on these. Let me know if I need to do anything to help. Thanks! Mark
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index e03df2881dc6..c5645331cfac 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -66,6 +66,7 @@ #include <linux/acpi.h> #include <linux/pci.h> #include <linux/power_supply.h> +#include <linux/platform_profile.h> #include <sound/core.h> #include <sound/control.h> #include <sound/initval.h> @@ -9854,16 +9855,27 @@ static bool has_lapsensor; static bool palm_state; static bool lap_state; -static int lapsensor_get(bool *present, bool *state) +static int dytc_command(int command, int *output) { acpi_handle dytc_handle; - int output; - *present = false; - if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) + if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DYTC", &dytc_handle))) { + /* Platform doesn't support DYTC */ return -ENODEV; - if (!acpi_evalf(dytc_handle, &output, NULL, "dd", DYTC_CMD_GET)) + } + if (!acpi_evalf(dytc_handle, output, NULL, "dd", command)) return -EIO; + return 0; +} + +static int lapsensor_get(bool *present, bool *state) +{ + int output, err; + + *present = false; + err = dytc_command(DYTC_CMD_GET, &output); + if (err) + return err; *present = true; /*If we get his far, we have lapmode support*/ *state = output & BIT(DYTC_GET_LAPMODE_BIT) ? true : false; @@ -9982,6 +9994,264 @@ static struct ibm_struct proxsensor_driver_data = { .exit = proxsensor_exit, }; +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) + +/************************************************************************* + * DYTC Platform Profile interface + */ + +#define DYTC_CMD_QUERY 0 /* To get DYTC status - enable/revision */ +#define DYTC_CMD_SET 1 /* To enable/disable IC function mode */ +#define DYTC_CMD_RESET 0x1ff /* To reset back to default */ + +#define DYTC_QUERY_ENABLE_BIT 8 /* Bit 8 - 0 = disabled, 1 = enabled */ +#define DYTC_QUERY_SUBREV_BIT 16 /* Bits 16 - 27 - sub revision */ +#define DYTC_QUERY_REV_BIT 28 /* Bits 28 - 31 - revision */ + +#define DYTC_GET_FUNCTION_BIT 8 /* Bits 8-11 - function setting */ +#define DYTC_GET_MODE_BIT 12 /* Bits 12-15 - mode setting */ + +#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - function setting */ +#define DYTC_SET_MODE_BIT 16 /* Bits 16-19 - mode setting */ +#define DYTC_SET_VALID_BIT 20 /* Bit 20 - 1 = on, 0 = off */ + +#define DYTC_FUNCTION_STD 0 /* Function = 0, standard mode */ +#define DYTC_FUNCTION_CQL 1 /* Function = 1, lap mode */ +#define DYTC_FUNCTION_MMC 11 /* Function = 11, desk mode */ + +#define DYTC_MODE_PERFORM 2 /* High power mode aka performance */ +#define DYTC_MODE_LOWPOWER 3 /* Low power mode */ +#define DYTC_MODE_BALANCE 0xF /* Default mode aka balanced */ + +#define DYTC_SET_COMMAND(function, mode, on) \ + (DYTC_CMD_SET | (function) << DYTC_SET_FUNCTION_BIT | \ + (mode) << DYTC_SET_MODE_BIT | \ + (on) << DYTC_SET_VALID_BIT) + +#define DYTC_DISABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 0) + +#define DYTC_ENABLE_CQL DYTC_SET_COMMAND(DYTC_FUNCTION_CQL, DYTC_MODE_BALANCE, 1) + +static bool dytc_profile_available; +static enum platform_profile_option dytc_current_profile; +static atomic_t dytc_ignore_event = ATOMIC_INIT(0); +static DEFINE_MUTEX(dytc_mutex); + +static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile) +{ + switch (dytcmode) { + case DYTC_MODE_LOWPOWER: + *profile = PLATFORM_PROFILE_LOW_POWER; + break; + case DYTC_MODE_BALANCE: + *profile = PLATFORM_PROFILE_BALANCED; + break; + case DYTC_MODE_PERFORM: + *profile = PLATFORM_PROFILE_PERFORMANCE; + break; + default: /* Unknown mode */ + return -EINVAL; + } + return 0; +} + +static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode) +{ + switch (profile) { + case PLATFORM_PROFILE_LOW_POWER: + *perfmode = DYTC_MODE_LOWPOWER; + break; + case PLATFORM_PROFILE_BALANCED: + *perfmode = DYTC_MODE_BALANCE; + break; + case PLATFORM_PROFILE_PERFORMANCE: + *perfmode = DYTC_MODE_PERFORM; + break; + default: /* Unknown profile */ + return -EOPNOTSUPP; + } + return 0; +} + +/* + * dytc_profile_get: Function to register with platform_profile + * handler. Returns current platform profile. + */ +int dytc_profile_get(struct platform_profile_handler *pprof, + enum platform_profile_option *profile) +{ + *profile = dytc_current_profile; + return 0; +} + +/* + * Helper function - check if we are in CQL mode and if we are + * - disable CQL, + * - run the command + * - enable CQL + * If not in CQL mode, just run the command + */ +int dytc_cql_command(int command, int *output) +{ + int err, cmd_err, dummy; + int cur_funcmode; + + /* Determine if we are in CQL mode. This alters the commands we do */ + err = dytc_command(DYTC_CMD_GET, output); + if (err) + return err; + + cur_funcmode = (*output >> DYTC_GET_FUNCTION_BIT) & 0xF; + /* Check if we're OK to return immediately */ + if ((command == DYTC_CMD_GET) && (cur_funcmode != DYTC_FUNCTION_CQL)) + return 0; + + if (cur_funcmode == DYTC_FUNCTION_CQL) { + atomic_inc(&dytc_ignore_event); + err = dytc_command(DYTC_DISABLE_CQL, &dummy); + if (err) + return err; + } + + cmd_err = dytc_command(command, output); + /* Check return condition after we've restored CQL state */ + + if (cur_funcmode == DYTC_FUNCTION_CQL) { + err = dytc_command(DYTC_ENABLE_CQL, &dummy); + if (err) + return err; + } + + return cmd_err; +} + +/* + * dytc_profile_set: Function to register with platform_profile + * handler. Sets current platform profile. + */ +int dytc_profile_set(struct platform_profile_handler *pprof, + enum platform_profile_option profile) +{ + int output; + int err; + + if (!dytc_profile_available) + return -ENODEV; + + err = mutex_lock_interruptible(&dytc_mutex); + if (err) + return err; + + if (profile == PLATFORM_PROFILE_BALANCED) { + /* To get back to balanced mode we just issue a reset command */ + err = dytc_command(DYTC_CMD_RESET, &output); + if (err) + goto unlock; + } else { + int perfmode; + + err = convert_profile_to_dytc(profile, &perfmode); + if (err) + goto unlock; + + /* Determine if we are in CQL mode. This alters the commands we do */ + err = dytc_cql_command(DYTC_SET_COMMAND(DYTC_FUNCTION_MMC, perfmode, 1), &output); + if (err) + goto unlock; + } + /* Success - update current profile */ + dytc_current_profile = profile; +unlock: + mutex_unlock(&dytc_mutex); + return err; +} + +static void dytc_profile_refresh(void) +{ + enum platform_profile_option profile; + int output, err; + int perfmode; + + mutex_lock(&dytc_mutex); + err = dytc_cql_command(DYTC_CMD_GET, &output); + mutex_unlock(&dytc_mutex); + if (err) + return; + + perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF; + convert_dytc_to_profile(perfmode, &profile); + if (profile != dytc_current_profile) { + dytc_current_profile = profile; + platform_profile_notify(); + } +} + +static struct platform_profile_handler dytc_profile = { + .profile_get = dytc_profile_get, + .profile_set = dytc_profile_set, +}; + +static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) +{ + int err, output; + + /* Setup supported modes */ + set_bit(PLATFORM_PROFILE_LOW_POWER, dytc_profile.choices); + set_bit(PLATFORM_PROFILE_BALANCED, dytc_profile.choices); + set_bit(PLATFORM_PROFILE_PERFORMANCE, dytc_profile.choices); + + dytc_profile_available = false; + err = dytc_command(DYTC_CMD_QUERY, &output); + /* + * If support isn't available (ENODEV) then don't return an error + * and don't create the sysfs group + */ + if (err == -ENODEV) + return 0; + /* For all other errors we can flag the failure */ + if (err) + return err; + + /* Check DYTC is enabled and supports mode setting */ + if (output & BIT(DYTC_QUERY_ENABLE_BIT)) { + /* Only DYTC v5.0 and later has this feature. */ + int dytc_version; + + dytc_version = (output >> DYTC_QUERY_REV_BIT) & 0xF; + if (dytc_version >= 5) { + dbg_printk(TPACPI_DBG_INIT, + "DYTC version %d: thermal mode available\n", dytc_version); + /* Create platform_profile structure and register */ + err = platform_profile_register(&dytc_profile); + /* + * If for some reason platform_profiles aren't enabled + * don't quit terminally. + */ + if (err) + return 0; + + dytc_profile_available = true; + /* Ensure initial values are correct */ + dytc_profile_refresh(); + } + } + return 0; +} + +static void dytc_profile_exit(void) +{ + if (dytc_profile_available) { + dytc_profile_available = false; + platform_profile_remove(); + } +} + +static struct ibm_struct dytc_profile_driver_data = { + .name = "dytc-profile", + .exit = dytc_profile_exit, +}; +#endif /* CONFIG_ACPI_PLATFORM_PROFILE */ + /**************************************************************************** **************************************************************************** * @@ -10030,8 +10300,14 @@ static void tpacpi_driver_event(const unsigned int hkey_event) mutex_unlock(&kbdlight_mutex); } - if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) + if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) { lapsensor_refresh(); +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) + /* If we are already accessing DYTC then skip dytc update */ + if (!atomic_add_unless(&dytc_ignore_event, -1, 0)) + dytc_profile_refresh(); +#endif + } } static void hotkey_driver_event(const unsigned int scancode) @@ -10474,6 +10750,12 @@ static struct ibm_init_struct ibms_init[] __initdata = { .init = tpacpi_proxsensor_init, .data = &proxsensor_driver_data, }, +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) + { + .init = tpacpi_dytc_profile_init, + .data = &dytc_profile_driver_data, + }, +#endif }; static int __init set_ibm_param(const char *val, const struct kernel_param *kp)
Add support to thinkpad_acpi for Lenovo platforms that have DYTC version 5 support or newer to use the platform profile feature. This will allow users to determine and control the platform modes between low-power, balanced operation and performance modes. Signed-off-by: Mark Pearson <markpearson@lenovo.com> --- Changes in v2: Address (hopefully) all recommendations from review including: - use IS_ENABLED instead of IS_DEFINED - update driver to work with all the fixes in platform_profile update - improve error handling for invalid inputs - move tracking of current profile mode into this driver Changes in v3: - version update for patch series Changes in v4: - Rebase on top of palm sensor patch which led to a little bit of file restructuring/clean up - Use BIT macro where applicable - Formatting fixes - Check sysfs node created on exit function - implement and use DYTC_SET_COMMAND macro - in case of failure setting performance mode make sure CQL mode is enabled again before returning. - Clean up initialisation and error handling code Changes in v5: - Use atomic_int with ignoring events - Add mutex when accessing ACPI information - Clean up registration code - Use cached value in profile_get function - Add dytc_cql_command function to clean up and provide common handler - Update to work with changes in platform_profile implementation Changes in v6: - Update file to build against update in v6 platform_profile Changes in v7 & v8: - version bump along with rest of patch series Changes in v9: - Update define names to be correct with the linux-pm branches accepted version - Rename DYTC_MODE_QUIET to LOW_POWER just because it's confusing. It was originally based on internal documentation. Note - originally this patch was part of a series but with the other two pieces being accepted into the acpi maintainers branch this one becomes standalone. drivers/platform/x86/thinkpad_acpi.c | 294 ++++++++++++++++++++++++++- 1 file changed, 288 insertions(+), 6 deletions(-)