| Message ID | 20201126165143.32776-3-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v4,1/3] Documentation: Add documentation for new platform_profile sysfs attribute | expand |
Hi 2020. november 26., csütörtök 17:51 keltezéssel, Mark Pearson írta: > [...] > +static bool dytc_ignore_next_event; As a sidenote: can the profile switching be triggered by means that's not the `/sys/firmware/acpi/platform_profile` attribute (e.g. a physical button)? Because if so, then I'm not sure if `dytc_ignore_next_event` achieves its purpose robustly, although I believe it won't cause issues in practice. I think it could be made more robust using a mutex to serialize and synchronize access to the DYTC interface, but I'm not sure if the effort is worth it. > +static bool dytc_profile_available; > +static enum platform_profile_option dytc_current_profile; > [...] > +static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) > +{ > + int err, output; > + > + dytc_profile_available = false; > + dytc_ignore_next_event = 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 */ > + do { > + err = platform_profile_register(&dytc_profile); > + } while (err == -EINTR); > [...] I'm wondering if this loop is really necessary? Regards, Barnabás Pőcze
Hi, On 11/26/20 5:51 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> > --- > 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 > > drivers/platform/x86/thinkpad_acpi.c | 306 ++++++++++++++++++++++++++- > 1 file changed, 305 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index 6a4c54db38fb..8463170391f5 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -72,6 +72,7 @@ > #include <linux/uaccess.h> > #include <acpi/battery.h> > #include <acpi/video.h> > +#include <linux/platform_profile.h> Please group this together with the other linux/foo.h includes. > > /* ThinkPad CMOS commands */ > #define TP_CMOS_VOLUME_DOWN 0 > @@ -9971,6 +9972,296 @@ 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_QUIET 3 /* Low power mode aka quiet */ > +#define DYTC_MODE_BALANCE 0xF /* Default mode aka balance */ > + > +#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_ignore_next_event; > +static bool dytc_profile_available; > +static enum platform_profile_option dytc_current_profile; > + > +static int dytc_command(int command, int *output) > +{ > + acpi_handle 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", command)) > + return -EIO; > + return 0; > +} > + > +static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile) > +{ > + switch (dytcmode) { > + case DYTC_MODE_QUIET: > + *profile = platform_profile_low; > + break; > + case DYTC_MODE_BALANCE: > + *profile = platform_profile_balance; > + break; > + case DYTC_MODE_PERFORM: > + *profile = platform_profile_perform; > + 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: > + *perfmode = DYTC_MODE_QUIET; > + break; > + case platform_profile_balance: > + *perfmode = DYTC_MODE_BALANCE; > + break; > + case platform_profile_perform: > + *perfmode = DYTC_MODE_PERFORM; > + break; > + default: /* Unknown profile */ > + return -EOPNOTSUPP; > + } > + return 0; > +} > + > +static int dytc_perfmode_get(int *perfmode, int *funcmode) > +{ > + int output, err, cmd_err; > + > + if (!dytc_profile_available) > + return -ENODEV; > + > + err = dytc_command(DYTC_CMD_GET, &output); > + if (err) > + return err; > + > + *funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF; > + if (*funcmode == DYTC_FUNCTION_CQL) { > + int dummy; > + /* > + * We can't get the mode when in CQL mode - so we disable CQL > + * mode retrieve the mode and then enable it again. > + * As disabling/enabling CQL triggers an event we set a flag to > + * ignore these events. This will be cleared by the event handler > + */ > + dytc_ignore_next_event = true; > + err = dytc_command(DYTC_DISABLE_CQL, &dummy); > + if (err) > + return err; > + > + cmd_err = dytc_command(DYTC_CMD_GET, &output); > + /* Check return condition after we've restored CQL state */ > + > + /* Again ignore this event */ > + dytc_ignore_next_event = true; Are we sure the event-handler will have run before we do this second setting of the ignore_next_event bool? Maybe make it an atomic integer and increment / decrement the variable ? E.g.: Declaration: static atomic_t dytc_ignore_next_event = ATOMIC_INIT(); Ignore next event: atomic_inc(&dytc_ignore_next_event); Check if event should be ignored: if (!atomic_add_unless(&dytc_ignore_next_event, -1, 0)) dytc_profile_refresh(); Note atomic_add_unless may needs some explanation, it adds -1 unless the atomic_t already contains 0. And it returns true if the addition was done. so if it returns true then dytc_ignore_next_event was not 0 (it might be zero afterwards). IOW if atomic_add_unless returns true then dytc_ignore_next_event was true, so then we should NOT continue with the refresh. > + err = dytc_command(DYTC_ENABLE_CQL, &dummy); > + if (err) > + return err; > + if (cmd_err) > + return cmd_err; > + } > + *perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF; > + return 0; > +} > + > +/* > + * dytc_profile_get: Function to register with platform_profile > + * handler. Returns current platform profile. > + */ > +int dytc_profile_get(enum platform_profile_option *profile) > +{ > + int funcmode, perfmode; > + int err; > + > + err = dytc_perfmode_get(&perfmode, &funcmode); > + if (err) > + return err; Can't we used a cached value here ? I presume we get an event when this is changed by the hotkey ? Esp. with the whole enable/disable CQL dance getting the value seems a bit expensive, so using a cached value might be better? > + > + /* Convert Lenovo DYTC profile to platform_profile */ > + err = convert_dytc_to_profile(perfmode, profile); > + if (err) > + return err; > + > + dytc_current_profile = *profile; > + return 0; > +} > + > +/* > + * dytc_profile_set: Function to register with platform_profile > + * handler. Sets current platform profile. > + */ > +int dytc_profile_set(enum platform_profile_option profile) > +{ > + int cur_perfmode, cur_funcmode; > + int output; > + int err; > + > + if (!dytc_profile_available) > + return -ENODEV; > + > + if (profile == platform_profile_balance) { > + /* To get back to balance mode we just issue a reset command */ > + err = dytc_command(DYTC_CMD_RESET, &output); > + if (err) > + return err; > + } else { > + int perfmode; > + int cmd_err; > + > + err = convert_profile_to_dytc(profile, &perfmode); > + if (err) > + return err; > + > + /* Determine if we are in CQL mode. This alters the commands we do */ > + err = dytc_perfmode_get(&cur_perfmode, &cur_funcmode); > + if (err) > + return err; > + > + if (cur_funcmode == DYTC_FUNCTION_CQL) { > + /* To set the mode we need to disable CQL first*/ > + dytc_ignore_next_event = true; /* Ignore event */ > + err = dytc_command(DYTC_DISABLE_CQL, &output); > + if (err) > + return err; > + } This seems somewhat duplicated from the get() code-path. Also you already doing a DYTC_DISABLE_CQL and DYTC_ENABLE_CQL in dytc_perfmode_get, which is not necessary to just get the funcmode which is all you need here AFAICT. IOW it seems that when CQL is active you are now doing: 1. dytc_perfmode_get() calls DYTC_CMD_GET 2. dytc_perfmode_get() calls DYTC_DISABLE_CQL 3. dytc_perfmode_get() calls DYTC_CMD_GET again, result is ignored (not used by dytc_profile_set) 4. dytc_perfmode_get() calls DYTC_ENABLE_CQL 5. dytc_profile_set() calls DYTC_DISABLE_CQL 6. dytc_profile_set() calls DYTC_SET_COMMAND 7. dytc_profile_set() calls DYTC_ENABLE_CQL And you can really skip step 2-4 here. I think it would be good to add a bunch of helpers: 1. dytc_get_modes() -> DYTC_CMD_GET wrapper gets both modes, sets perfmode to -1 when funcmode is CQL 2. dytc_disable_cql_if_necessary() which takes funcmode as argument and is a no-op when funcmode != CQL 3. dytc_re_enable_cql_if_necessary() idem. And then the flow in dytc_perfmode_get could look something like this (pseudo code minus error handling): dytc_get_modes(&funcmode, &perfmode) if (funcmode != CQL) /* or alternatively check for perfmode != -1 */ return success; dytc_disable_cql_if_necessary(funcmode); dytc_get_modes(NULL, &perfmode); dytc_disable_cql_if_necessary(funcmode); And in the non-balanced path of dytc_profile_set: dytc_get_modes(&funcmode, NULL) dytc_disable_cql_if_necessary(funcmode); dytc_set_mode(...); dytc_disable_cql_if_necessary(funcmode); Note the NULL could be a dummy, but I find NULL a bit cleaner (at the cost of having to check for it in dytc_get_modes). This is is just from a quick peek, when you implement this it might turn out to be less then ideal, IOW this is just a suggestion, feel free to deviate. ### Since this will require a bit of work, timing wise (wrt the 5.11 merge-window) it might be best to just keep this patch as is for v5, and only change patch 1 and 2 of the set, so that those will hopefully be ready for merging in time for the 5.11 window. I plan to pick this one up once 5.11-rc1 is out (and has the necessary ACPI bits) so we have some more time to get this one in shape. For patch 1/2 the most important thing is to have a consumer of the new internal APIs (almost) ready and this code fulfills that in its current form. Regards, Hans > + cmd_err = dytc_command(DYTC_SET_COMMAND(DYTC_FUNCTION_MMC, perfmode, 1), > + &output); > + /* Check return condition after we've restored CQL state */ > + > + if (cur_funcmode == DYTC_FUNCTION_CQL) { > + dytc_ignore_next_event = true; /* Ignore event */ > + err = dytc_command(DYTC_ENABLE_CQL, &output); > + if (err) > + return err; > + } > + if (cmd_err) > + return cmd_err; > + } > + /* Success - update current profile */ > + dytc_current_profile = profile; > + return 0; > +} > + > +static void dytc_profile_refresh(void) > +{ > + enum platform_profile_option profile; > + int perfmode, funcmode; > + int err; > + > + err = dytc_perfmode_get(&perfmode, &funcmode); > + if (err) > + return; > + > + err = 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 = { > + .choices = BIT(platform_profile_low) | > + BIT(platform_profile_balance) | > + BIT(platform_profile_perform), > + .profile_get = dytc_profile_get, > + .profile_set = dytc_profile_set, > +}; > + > +static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) > +{ > + int err, output; > + > + dytc_profile_available = false; > + dytc_ignore_next_event = 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 */ > + do { > + err = platform_profile_register(&dytc_profile); > + } while (err == -EINTR); > + /* > + * If for some reason platform_profiles aren't enabled > + * don't quit terminally. > + */ > + if (err) > + return 0; > + dytc_profile_available = true; > + } > + } > + return 0; > +} > + > +static void dytc_profile_exit(void) > +{ > + if (dytc_profile_available) { > + dytc_profile_available = false; > + platform_profile_unregister(); > + } > +} > + > +static struct ibm_struct dytc_profile_driver_data = { > + .name = "dytc-profile", > + .exit = dytc_profile_exit, > +}; > +#endif /* CONFIG_ACPI_PLATFORM_PROFILE */ > + > /**************************************************************************** > **************************************************************************** > * > @@ -10019,8 +10310,15 @@ 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 (dytc_ignore_next_event) > + dytc_ignore_next_event = false; /*clear setting*/ > + else > + dytc_profile_refresh(); > +#endif > + } > } > > static void hotkey_driver_event(const unsigned int scancode) > @@ -10463,6 +10761,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) >
Hi, On 11/27/20 8:22 PM, Barnabás Pőcze wrote: > Hi > > > 2020. november 26., csütörtök 17:51 keltezéssel, Mark Pearson írta: > >> [...] >> +static bool dytc_ignore_next_event; > > As a sidenote: can the profile switching be triggered by means that's not the > `/sys/firmware/acpi/platform_profile` attribute (e.g. a physical button)? > Because if so, then I'm not sure if `dytc_ignore_next_event` achieves its purpose > robustly, although I believe it won't cause issues in practice. I think it could > be made more robust using a mutex to serialize and synchronize access to the DYTC > interface, but I'm not sure if the effort is worth it. > > >> +static bool dytc_profile_available; >> +static enum platform_profile_option dytc_current_profile; >> [...] >> +static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) >> +{ >> + int err, output; >> + >> + dytc_profile_available = false; >> + dytc_ignore_next_event = 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 */ >> + do { >> + err = platform_profile_register(&dytc_profile); >> + } while (err == -EINTR); >> [...] > > I'm wondering if this loop is really necessary? It is the result of using mutex_interruptible inside platform_profile_register(), once that is fixed (as I just requested in my review of patch 2/3) then this loop can go away. Regards, Hans
Hi 2020. november 28., szombat 16:00 keltezéssel, Hans de Goede <hdegoede@redhat.com> írta: > [...] > >> +static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) > >> +{ > >> + int err, output; > >> + > >> + dytc_profile_available = false; > >> + dytc_ignore_next_event = 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 */ > >> + do { > >> + err = platform_profile_register(&dytc_profile); > >> + } while (err == -EINTR); > >> [...] > > > > I'm wondering if this loop is really necessary? > > It is the result of using mutex_interruptible inside platform_profile_register(), > once that is fixed (as I just requested in my review of patch 2/3) then this loop > can go away. > Thank you, I see that, my question should've been "why not simply fail and return the error?", but with your requested change the question is moot. Regards, Barnabás Pőcze
Hi Barnabas, On 2020-11-27 2:22 p.m., Barnabás Pőcze wrote: > Hi > > > 2020. november 26., csütörtök 17:51 keltezéssel, Mark Pearson írta: > >> [...] >> +static bool dytc_ignore_next_event; > > As a sidenote: can the profile switching be triggered by means that's not the > `/sys/firmware/acpi/platform_profile` attribute (e.g. a physical button)? > Because if so, then I'm not sure if `dytc_ignore_next_event` achieves its purpose > robustly, although I believe it won't cause issues in practice. I think it could > be made more robust using a mutex to serialize and synchronize access to the DYTC > interface, but I'm not sure if the effort is worth it. > A user can do FN+L, FN+M, FN+H to switch mode (though hopefully with this API and the support in user space they won't need to do that any more). So I think you're right about this area having issues, and Hans picks up on this too. I had avoided a mutex as I thought that would cause problems in the event handler. In Han's email he suggests an atomic int and I think that could work nicely but will have to try it out and see. Regardless - I agree this area needs some work and I'll look into it Thanks! Mark
Hi Hans, Sorry for the slow reply on this one - I went and did some investigation/testing first (and the US came back from Thanksgiving with a vengence of meetings....) On 28/11/2020 09:55, Hans de Goede wrote: > Hi, > > On 11/26/20 5:51 PM, Mark Pearson wrote: <snip> >> >> drivers/platform/x86/thinkpad_acpi.c | 306 ++++++++++++++++++++++++++- >> 1 file changed, 305 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >> index 6a4c54db38fb..8463170391f5 100644 >> --- a/drivers/platform/x86/thinkpad_acpi.c >> +++ b/drivers/platform/x86/thinkpad_acpi.c >> @@ -72,6 +72,7 @@ >> #include <linux/uaccess.h> >> #include <acpi/battery.h> >> #include <acpi/video.h> >> +#include <linux/platform_profile.h> > > Please group this together with the other linux/foo.h includes. Ack. Is it OK if I tidy up the list to be alphabetical as seems generally preferred, or would you rather I didn't mess with it apart from the one small adjustment? > >> <snip> >> +} >> + >> +static int dytc_perfmode_get(int *perfmode, int *funcmode) >> +{ >> + int output, err, cmd_err; >> + >> + if (!dytc_profile_available) >> + return -ENODEV; >> + >> + err = dytc_command(DYTC_CMD_GET, &output); >> + if (err) >> + return err; >> + >> + *funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF; >> + if (*funcmode == DYTC_FUNCTION_CQL) { >> + int dummy; >> + /* >> + * We can't get the mode when in CQL mode - so we disable CQL >> + * mode retrieve the mode and then enable it again. >> + * As disabling/enabling CQL triggers an event we set a flag to >> + * ignore these events. This will be cleared by the event handler >> + */ >> + dytc_ignore_next_event = true; >> + err = dytc_command(DYTC_DISABLE_CQL, &dummy); >> + if (err) >> + return err; >> + >> + cmd_err = dytc_command(DYTC_CMD_GET, &output); >> + /* Check return condition after we've restored CQL state */ >> + >> + /* Again ignore this event */ >> + dytc_ignore_next_event = true; > > Are we sure the event-handler will have run before we do this second > setting of the ignore_next_event bool? Maybe make it an atomic integer > and increment / decrement the variable ? > > E.g.: > > Declaration: > > static atomic_t dytc_ignore_next_event = ATOMIC_INIT(); > > Ignore next event: > atomic_inc(&dytc_ignore_next_event); > > Check if event should be ignored: > > if (!atomic_add_unless(&dytc_ignore_next_event, -1, 0)) > dytc_profile_refresh(); > > Note atomic_add_unless may needs some explanation, it adds -1 unless > the atomic_t already contains 0. And it returns true if the addition > was done. so if it returns true then dytc_ignore_next_event was not 0 > (it might be zero afterwards). > > IOW if atomic_add_unless returns true then dytc_ignore_next_event was true, > so then we should NOT continue with the refresh. > In my testing the event handler always ran first, but the atomic approach is much nicer - thank you for the suggestion. I've played a bit with this and tried a few things over the last few days and it has been working nicely One thing I noticed is I think I need to add a mutex to protect so that a FN key press won't interfere with a user space access and vice-versa. > > > >> + err = dytc_command(DYTC_ENABLE_CQL, &dummy); >> + if (err) >> + return err; >> + if (cmd_err) >> + return cmd_err; >> + } >> + *perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF; >> + return 0; >> +} >> + >> +/* >> + * dytc_profile_get: Function to register with platform_profile >> + * handler. Returns current platform profile. >> + */ >> +int dytc_profile_get(enum platform_profile_option *profile) >> +{ >> + int funcmode, perfmode; >> + int err; >> + >> + err = dytc_perfmode_get(&perfmode, &funcmode); >> + if (err) >> + return err; > > Can't we used a cached value here ? I presume we get an > event when this is changed by the hotkey ? Esp. with the > whole enable/disable CQL dance getting the value seems a > bit expensive, so using a cached value might be better? Agreed - I'll implement. > >> + >> + /* Convert Lenovo DYTC profile to platform_profile */ >> + err = convert_dytc_to_profile(perfmode, profile); >> + if (err) >> + return err; >> + >> + dytc_current_profile = *profile; >> + return 0; >> +} >> + >> +/* >> + * dytc_profile_set: Function to register with platform_profile >> + * handler. Sets current platform profile. >> + */ >> +int dytc_profile_set(enum platform_profile_option profile) >> +{ >> + int cur_perfmode, cur_funcmode; >> + int output; >> + int err; >> + >> + if (!dytc_profile_available) >> + return -ENODEV; >> + >> + if (profile == platform_profile_balance) { >> + /* To get back to balance mode we just issue a reset command */ >> + err = dytc_command(DYTC_CMD_RESET, &output); >> + if (err) >> + return err; >> + } else { >> + int perfmode; >> + int cmd_err; >> + >> + err = convert_profile_to_dytc(profile, &perfmode); >> + if (err) >> + return err; >> + >> + /* Determine if we are in CQL mode. This alters the commands we do */ >> + err = dytc_perfmode_get(&cur_perfmode, &cur_funcmode); >> + if (err) >> + return err; >> + >> + if (cur_funcmode == DYTC_FUNCTION_CQL) { >> + /* To set the mode we need to disable CQL first*/ >> + dytc_ignore_next_event = true; /* Ignore event */ >> + err = dytc_command(DYTC_DISABLE_CQL, &output); >> + if (err) >> + return err; >> + } > > This seems somewhat duplicated from the get() code-path. Also you already doing > a DYTC_DISABLE_CQL and DYTC_ENABLE_CQL in dytc_perfmode_get, which is not necessary > to just get the funcmode which is all you need here AFAICT. > > IOW it seems that when CQL is active you are now doing: > > 1. dytc_perfmode_get() calls DYTC_CMD_GET > 2. dytc_perfmode_get() calls DYTC_DISABLE_CQL > 3. dytc_perfmode_get() calls DYTC_CMD_GET again, result is ignored (not used by dytc_profile_set) > 4. dytc_perfmode_get() calls DYTC_ENABLE_CQL > 5. dytc_profile_set() calls DYTC_DISABLE_CQL > 6. dytc_profile_set() calls DYTC_SET_COMMAND > 7. dytc_profile_set() calls DYTC_ENABLE_CQL > > And you can really skip step 2-4 here. > > I think it would be good to add a bunch of helpers: > > 1. dytc_get_modes() -> DYTC_CMD_GET wrapper gets both modes, sets perfmode > to -1 when funcmode is CQL > 2. dytc_disable_cql_if_necessary() which takes funcmode as argument and is > a no-op when funcmode != CQL > 3. dytc_re_enable_cql_if_necessary() idem. > > And then the flow in dytc_perfmode_get could look something like this > (pseudo code minus error handling): > > dytc_get_modes(&funcmode, &perfmode) > if (funcmode != CQL) /* or alternatively check for perfmode != -1 */ > return success; > > dytc_disable_cql_if_necessary(funcmode); > dytc_get_modes(NULL, &perfmode); > dytc_disable_cql_if_necessary(funcmode); > > And in the non-balanced path of dytc_profile_set: > > dytc_get_modes(&funcmode, NULL) > > dytc_disable_cql_if_necessary(funcmode); > dytc_set_mode(...); > dytc_disable_cql_if_necessary(funcmode); > > Note the NULL could be a dummy, but I find NULL a bit cleaner > (at the cost of having to check for it in dytc_get_modes). > > This is is just from a quick peek, when you implement this > it might turn out to be less then ideal, IOW this is just > a suggestion, feel free to deviate. Agreed - and thank you for the suggestions. I did prototype a similar method and it has worked out nicely. I've got a bit more cleanup but the code is better than it was. > > ### > > Since this will require a bit of work, timing wise (wrt the 5.11 merge-window) > it might be best to just keep this patch as is for v5, and only change > patch 1 and 2 of the set, so that those will hopefully be ready for > merging in time for the 5.11 window. I plan to pick this one up > once 5.11-rc1 is out (and has the necessary ACPI bits) so we have some > more time to get this one in shape. > > For patch 1/2 the most important thing is to have a consumer of the > new internal APIs (almost) ready and this code fulfills that in > its current form. OK - I think that makes sense. Just curious though - will you then just accept the platform_profile pieces (1 & 2)? Would it make it easier if I just push the updated first two patches and drop thinkpad_acpi.c for now (it will follow shortly, but is going to be a couple more days) or would you rather have everything and just pick the bits you want? I've got the v5 ready (I think) for the platform profile and am still working on thinkpad_acpi.c with the improvements from above. I think I'll be a couple more days there. Thank you! Mark
Hi, On 12/1/20 5:51 PM, Mark Pearson wrote: > Hi Hans, > > Sorry for the slow reply on this one - I went and did some investigation/testing first (and the US came back from Thanksgiving with a vengence of meetings....) > > On 28/11/2020 09:55, Hans de Goede wrote: >> Hi, >> >> On 11/26/20 5:51 PM, Mark Pearson wrote: > <snip> >>> >>> drivers/platform/x86/thinkpad_acpi.c | 306 ++++++++++++++++++++++++++- >>> 1 file changed, 305 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >>> index 6a4c54db38fb..8463170391f5 100644 >>> --- a/drivers/platform/x86/thinkpad_acpi.c >>> +++ b/drivers/platform/x86/thinkpad_acpi.c >>> @@ -72,6 +72,7 @@ >>> #include <linux/uaccess.h> >>> #include <acpi/battery.h> >>> #include <acpi/video.h> >>> +#include <linux/platform_profile.h> >> >> Please group this together with the other linux/foo.h includes. > Ack. > > Is it OK if I tidy up the list to be alphabetical as seems generally preferred, or would you rather I didn't mess with it apart from the one small adjustment? I would welcome a *separate* patch to sort things alphabetically, either as a preparation or as a follow-up patch. But please don't combine that with this patch. >> >>> > <snip> >>> +} >>> + >>> +static int dytc_perfmode_get(int *perfmode, int *funcmode) >>> +{ >>> + int output, err, cmd_err; >>> + >>> + if (!dytc_profile_available) >>> + return -ENODEV; >>> + >>> + err = dytc_command(DYTC_CMD_GET, &output); >>> + if (err) >>> + return err; >>> + >>> + *funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF; >>> + if (*funcmode == DYTC_FUNCTION_CQL) { >>> + int dummy; >>> + /* >>> + * We can't get the mode when in CQL mode - so we disable CQL >>> + * mode retrieve the mode and then enable it again. >>> + * As disabling/enabling CQL triggers an event we set a flag to >>> + * ignore these events. This will be cleared by the event handler >>> + */ >>> + dytc_ignore_next_event = true; >>> + err = dytc_command(DYTC_DISABLE_CQL, &dummy); >>> + if (err) >>> + return err; >>> + >>> + cmd_err = dytc_command(DYTC_CMD_GET, &output); >>> + /* Check return condition after we've restored CQL state */ >>> + >>> + /* Again ignore this event */ >>> + dytc_ignore_next_event = true; >> >> Are we sure the event-handler will have run before we do this second >> setting of the ignore_next_event bool? Maybe make it an atomic integer >> and increment / decrement the variable ? >> >> E.g.: >> >> Declaration: >> >> static atomic_t dytc_ignore_next_event = ATOMIC_INIT(); >> >> Ignore next event: >> atomic_inc(&dytc_ignore_next_event); >> >> Check if event should be ignored: >> >> if (!atomic_add_unless(&dytc_ignore_next_event, -1, 0)) >> dytc_profile_refresh(); >> >> Note atomic_add_unless may needs some explanation, it adds -1 unless >> the atomic_t already contains 0. And it returns true if the addition >> was done. so if it returns true then dytc_ignore_next_event was not 0 >> (it might be zero afterwards). >> >> IOW if atomic_add_unless returns true then dytc_ignore_next_event was true, >> so then we should NOT continue with the refresh. >> > In my testing the event handler always ran first, but the atomic approach is much nicer - thank you for the suggestion. > I've played a bit with this and tried a few things over the last few days and it has been working nicely > One thing I noticed is I think I need to add a mutex to protect so that a FN key press won't interfere with a user space access and vice-versa. Ok, sounds good (adding a mutex if necessary is fine). > >> >> >> >>> + err = dytc_command(DYTC_ENABLE_CQL, &dummy); >>> + if (err) >>> + return err; >>> + if (cmd_err) >>> + return cmd_err; >>> + } >>> + *perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF; >>> + return 0; >>> +} >>> + >>> +/* >>> + * dytc_profile_get: Function to register with platform_profile >>> + * handler. Returns current platform profile. >>> + */ >>> +int dytc_profile_get(enum platform_profile_option *profile) >>> +{ >>> + int funcmode, perfmode; >>> + int err; >>> + >>> + err = dytc_perfmode_get(&perfmode, &funcmode); >>> + if (err) >>> + return err; >> >> Can't we used a cached value here ? I presume we get an >> event when this is changed by the hotkey ? Esp. with the >> whole enable/disable CQL dance getting the value seems a >> bit expensive, so using a cached value might be better? > > Agreed - I'll implement. >> >>> + >>> + /* Convert Lenovo DYTC profile to platform_profile */ >>> + err = convert_dytc_to_profile(perfmode, profile); >>> + if (err) >>> + return err; >>> + >>> + dytc_current_profile = *profile; >>> + return 0; >>> +} >>> + >>> +/* >>> + * dytc_profile_set: Function to register with platform_profile >>> + * handler. Sets current platform profile. >>> + */ >>> +int dytc_profile_set(enum platform_profile_option profile) >>> +{ >>> + int cur_perfmode, cur_funcmode; >>> + int output; >>> + int err; >>> + >>> + if (!dytc_profile_available) >>> + return -ENODEV; >>> + >>> + if (profile == platform_profile_balance) { >>> + /* To get back to balance mode we just issue a reset command */ >>> + err = dytc_command(DYTC_CMD_RESET, &output); >>> + if (err) >>> + return err; >>> + } else { >>> + int perfmode; >>> + int cmd_err; >>> + >>> + err = convert_profile_to_dytc(profile, &perfmode); >>> + if (err) >>> + return err; >>> + >>> + /* Determine if we are in CQL mode. This alters the commands we do */ >>> + err = dytc_perfmode_get(&cur_perfmode, &cur_funcmode); >>> + if (err) >>> + return err; >>> + >>> + if (cur_funcmode == DYTC_FUNCTION_CQL) { >>> + /* To set the mode we need to disable CQL first*/ >>> + dytc_ignore_next_event = true; /* Ignore event */ >>> + err = dytc_command(DYTC_DISABLE_CQL, &output); >>> + if (err) >>> + return err; >>> + } >> >> This seems somewhat duplicated from the get() code-path. Also you already doing >> a DYTC_DISABLE_CQL and DYTC_ENABLE_CQL in dytc_perfmode_get, which is not necessary >> to just get the funcmode which is all you need here AFAICT. >> >> IOW it seems that when CQL is active you are now doing: >> >> 1. dytc_perfmode_get() calls DYTC_CMD_GET >> 2. dytc_perfmode_get() calls DYTC_DISABLE_CQL >> 3. dytc_perfmode_get() calls DYTC_CMD_GET again, result is ignored (not used by dytc_profile_set) >> 4. dytc_perfmode_get() calls DYTC_ENABLE_CQL >> 5. dytc_profile_set() calls DYTC_DISABLE_CQL >> 6. dytc_profile_set() calls DYTC_SET_COMMAND >> 7. dytc_profile_set() calls DYTC_ENABLE_CQL >> >> And you can really skip step 2-4 here. >> >> I think it would be good to add a bunch of helpers: >> >> 1. dytc_get_modes() -> DYTC_CMD_GET wrapper gets both modes, sets perfmode >> to -1 when funcmode is CQL >> 2. dytc_disable_cql_if_necessary() which takes funcmode as argument and is >> a no-op when funcmode != CQL >> 3. dytc_re_enable_cql_if_necessary() idem. >> >> And then the flow in dytc_perfmode_get could look something like this >> (pseudo code minus error handling): >> >> dytc_get_modes(&funcmode, &perfmode) >> if (funcmode != CQL) /* or alternatively check for perfmode != -1 */ >> return success; >> >> dytc_disable_cql_if_necessary(funcmode); >> dytc_get_modes(NULL, &perfmode); >> dytc_disable_cql_if_necessary(funcmode); >> >> And in the non-balanced path of dytc_profile_set: >> >> dytc_get_modes(&funcmode, NULL) >> >> dytc_disable_cql_if_necessary(funcmode); >> dytc_set_mode(...); >> dytc_disable_cql_if_necessary(funcmode); >> >> Note the NULL could be a dummy, but I find NULL a bit cleaner >> (at the cost of having to check for it in dytc_get_modes). >> >> This is is just from a quick peek, when you implement this >> it might turn out to be less then ideal, IOW this is just >> a suggestion, feel free to deviate. > > Agreed - and thank you for the suggestions. I did prototype a similar method and it has worked out nicely. I've got a bit more cleanup but the code is better than it was. >> >> ### >> >> Since this will require a bit of work, timing wise (wrt the 5.11 merge-window) >> it might be best to just keep this patch as is for v5, and only change >> patch 1 and 2 of the set, so that those will hopefully be ready for >> merging in time for the 5.11 window. I plan to pick this one up >> once 5.11-rc1 is out (and has the necessary ACPI bits) so we have some >> more time to get this one in shape. >> >> For patch 1/2 the most important thing is to have a consumer of the >> new internal APIs (almost) ready and this code fulfills that in >> its current form. > > OK - I think that makes sense. Just curious though - will you then just accept the platform_profile pieces (1 & 2)? Would it make it easier if I just push the updated first two patches and drop thinkpad_acpi.c for now (it will follow shortly, but is going to be a couple more days) or would you rather have everything and just pick the bits you want? Patches 1 & 2 should be merged by Rafael, who maintains drivers/acpi, normally I would then ask Rafael for an immutable branch with those bits and merge that into platform-drivers-x86.git/for-next and then merge the 3th patch there. But given the timing it will be easier to just wait for 5.11-rc1, assuming Rafael is still willing to take 1 and 2 as 5.11 material. The time window for that is closing. Rafael, would you be willing to take patches 1 and 2 of this series as 5.11 material assuming a new version addressing my review remarks get posted soon and I then give my Reviewed-by ? > I've got the v5 ready (I think) for the platform profile and am still working on thinkpad_acpi.c with the improvements from above. I think I'll be a couple more days there. It would be best if you can send out v5 soon, even if the thinkpad_acpi patch is not in perfect shape yet, it will still illustrate how the new internal APIs from patch 2 will be used, which is very useful for reviewing patch 2. Regards, Hans
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 6a4c54db38fb..8463170391f5 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -72,6 +72,7 @@ #include <linux/uaccess.h> #include <acpi/battery.h> #include <acpi/video.h> +#include <linux/platform_profile.h> /* ThinkPad CMOS commands */ #define TP_CMOS_VOLUME_DOWN 0 @@ -9971,6 +9972,296 @@ 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_QUIET 3 /* Low power mode aka quiet */ +#define DYTC_MODE_BALANCE 0xF /* Default mode aka balance */ + +#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_ignore_next_event; +static bool dytc_profile_available; +static enum platform_profile_option dytc_current_profile; + +static int dytc_command(int command, int *output) +{ + acpi_handle 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", command)) + return -EIO; + return 0; +} + +static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile) +{ + switch (dytcmode) { + case DYTC_MODE_QUIET: + *profile = platform_profile_low; + break; + case DYTC_MODE_BALANCE: + *profile = platform_profile_balance; + break; + case DYTC_MODE_PERFORM: + *profile = platform_profile_perform; + 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: + *perfmode = DYTC_MODE_QUIET; + break; + case platform_profile_balance: + *perfmode = DYTC_MODE_BALANCE; + break; + case platform_profile_perform: + *perfmode = DYTC_MODE_PERFORM; + break; + default: /* Unknown profile */ + return -EOPNOTSUPP; + } + return 0; +} + +static int dytc_perfmode_get(int *perfmode, int *funcmode) +{ + int output, err, cmd_err; + + if (!dytc_profile_available) + return -ENODEV; + + err = dytc_command(DYTC_CMD_GET, &output); + if (err) + return err; + + *funcmode = (output >> DYTC_GET_FUNCTION_BIT) & 0xF; + if (*funcmode == DYTC_FUNCTION_CQL) { + int dummy; + /* + * We can't get the mode when in CQL mode - so we disable CQL + * mode retrieve the mode and then enable it again. + * As disabling/enabling CQL triggers an event we set a flag to + * ignore these events. This will be cleared by the event handler + */ + dytc_ignore_next_event = true; + err = dytc_command(DYTC_DISABLE_CQL, &dummy); + if (err) + return err; + + cmd_err = dytc_command(DYTC_CMD_GET, &output); + /* Check return condition after we've restored CQL state */ + + /* Again ignore this event */ + dytc_ignore_next_event = true; + err = dytc_command(DYTC_ENABLE_CQL, &dummy); + if (err) + return err; + if (cmd_err) + return cmd_err; + } + *perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF; + return 0; +} + +/* + * dytc_profile_get: Function to register with platform_profile + * handler. Returns current platform profile. + */ +int dytc_profile_get(enum platform_profile_option *profile) +{ + int funcmode, perfmode; + int err; + + err = dytc_perfmode_get(&perfmode, &funcmode); + if (err) + return err; + + /* Convert Lenovo DYTC profile to platform_profile */ + err = convert_dytc_to_profile(perfmode, profile); + if (err) + return err; + + dytc_current_profile = *profile; + return 0; +} + +/* + * dytc_profile_set: Function to register with platform_profile + * handler. Sets current platform profile. + */ +int dytc_profile_set(enum platform_profile_option profile) +{ + int cur_perfmode, cur_funcmode; + int output; + int err; + + if (!dytc_profile_available) + return -ENODEV; + + if (profile == platform_profile_balance) { + /* To get back to balance mode we just issue a reset command */ + err = dytc_command(DYTC_CMD_RESET, &output); + if (err) + return err; + } else { + int perfmode; + int cmd_err; + + err = convert_profile_to_dytc(profile, &perfmode); + if (err) + return err; + + /* Determine if we are in CQL mode. This alters the commands we do */ + err = dytc_perfmode_get(&cur_perfmode, &cur_funcmode); + if (err) + return err; + + if (cur_funcmode == DYTC_FUNCTION_CQL) { + /* To set the mode we need to disable CQL first*/ + dytc_ignore_next_event = true; /* Ignore event */ + err = dytc_command(DYTC_DISABLE_CQL, &output); + if (err) + return err; + } + cmd_err = dytc_command(DYTC_SET_COMMAND(DYTC_FUNCTION_MMC, perfmode, 1), + &output); + /* Check return condition after we've restored CQL state */ + + if (cur_funcmode == DYTC_FUNCTION_CQL) { + dytc_ignore_next_event = true; /* Ignore event */ + err = dytc_command(DYTC_ENABLE_CQL, &output); + if (err) + return err; + } + if (cmd_err) + return cmd_err; + } + /* Success - update current profile */ + dytc_current_profile = profile; + return 0; +} + +static void dytc_profile_refresh(void) +{ + enum platform_profile_option profile; + int perfmode, funcmode; + int err; + + err = dytc_perfmode_get(&perfmode, &funcmode); + if (err) + return; + + err = 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 = { + .choices = BIT(platform_profile_low) | + BIT(platform_profile_balance) | + BIT(platform_profile_perform), + .profile_get = dytc_profile_get, + .profile_set = dytc_profile_set, +}; + +static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm) +{ + int err, output; + + dytc_profile_available = false; + dytc_ignore_next_event = 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 */ + do { + err = platform_profile_register(&dytc_profile); + } while (err == -EINTR); + /* + * If for some reason platform_profiles aren't enabled + * don't quit terminally. + */ + if (err) + return 0; + dytc_profile_available = true; + } + } + return 0; +} + +static void dytc_profile_exit(void) +{ + if (dytc_profile_available) { + dytc_profile_available = false; + platform_profile_unregister(); + } +} + +static struct ibm_struct dytc_profile_driver_data = { + .name = "dytc-profile", + .exit = dytc_profile_exit, +}; +#endif /* CONFIG_ACPI_PLATFORM_PROFILE */ + /**************************************************************************** **************************************************************************** * @@ -10019,8 +10310,15 @@ 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 (dytc_ignore_next_event) + dytc_ignore_next_event = false; /*clear setting*/ + else + dytc_profile_refresh(); +#endif + } } static void hotkey_driver_event(const unsigned int scancode) @@ -10463,6 +10761,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 drivers/platform/x86/thinkpad_acpi.c | 306 ++++++++++++++++++++++++++- 1 file changed, 305 insertions(+), 1 deletion(-)