| Message ID | 20201114150102.340618-3-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/3] Documentation: Add documentation for new platform_profile sysfs attribute | expand |
2020. november 14., szombat 16:01 keltezéssel, Mark Pearson <markpearson@lenovo.com> írta: Hi I think there are a couple places where the BIT() macro could be used. > [...] > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index 890dda284a00..13352ccdfdaf 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 > @@ -9832,10 +9833,40 @@ static struct ibm_struct lcdshadow_driver_data = { > * DYTC subdriver, for the Lenovo lapmode feature This comment should be updated, no? It does more than report the "lap mode" state? > */ > > +#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_GET 2 /* To get current IC function and mode */ > #define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */ `DYTC_GET_LAPMODE_BIT` is defined a couple lines below, I think this definition should be removed. > +#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 revisision */ ^ "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 */ ^ The spacing is inconsistent in the comments. | > +#define DYTC_GET_LAPMODE_BIT 17 /* Bit 17 - lapmode. Set when on lap */ > + > +#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - funct setting */ If all letters of "function" were spelled a couple lines above, I think it should be here as well. > +#define DYTC_SET_MODE_BIT 16 /* Bits 16-19 - mode setting */ > +#define DYTC_SET_VALID_BIT 20 /* Bit 20 - 1 = on, 0 = off */ > + I personally would create a DYTC_SET_COMMAND() - or similarly named - macro along these lines: ``` #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) ``` and use that later on. I believe this helps readability and reduces the chances of accindental mistakes. > +#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 */ It seems strange to me that there is a leap from 1 to 11. > + > +#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 */ I suggest you capitalize the last two comments to be consistent with the rest of the patch. > + > +#define DYTC_DISABLE_CQL ((DYTC_MODE_BALANCE << DYTC_SET_MODE_BIT) | \ > + (DYTC_FUNCTION_CQL << DYTC_SET_FUNCTION_BIT) | \ > + DYTC_CMD_SET) > +#define DYTC_ENABLE_CQL (DYTC_DISABLE_CQL | (1 << DYTC_SET_VALID_BIT)) > [...] > +static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode) > +{ > + switch (profile) { > + case profile_low: > + *perfmode = DYTC_MODE_QUIET; > + break; > + case profile_balance: > + *perfmode = DYTC_MODE_BALANCE; > + break; > + case profile_perform: > + *perfmode = DYTC_MODE_PERFORM; > + break; > + default: /* Unknown profile */ > + return -EOPNOTSUPP; I personally think EINVAL would be better here, just like in `convert_dytc_to_profile()`. > + } > + return 0; > +} > + > +static int dytc_perfmode_get(int *perfmode, int *funcmode) > +{ > + int output, err; > + > + if (!dytc_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; > + err = dytc_command(DYTC_CMD_GET, &output); > + if (err) > + return err; If `DYTC_CMD_GET` fails, then CQL state is not restored. I think that may be undesired? > + /* Again ignore this event */ > + dytc_ignore_next_event = true; > + err = dytc_command(DYTC_ENABLE_CQL, &dummy); > + if (err) > + return err; > + } > + *perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF; > + 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 err, dytc_set; > + int output; > + > + if (!dytc_available) > + return -ENODEV; > + > + if (profile == 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 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; > + } > + dytc_set = (1 << DYTC_SET_VALID_BIT) | > + (DYTC_FUNCTION_MMC << DYTC_SET_FUNCTION_BIT) | > + (perfmode << DYTC_SET_MODE_BIT) | > + DYTC_CMD_SET; > + err = dytc_command(dytc_set, &output); > + if (err) > + return err; If I see it correctly, if CQL is turned off successfully, but the this command fails, then the function returns with an error, but does not restore CQL state. Which may or may not be desired? > + if (cur_funcmode == DYTC_FUNCTION_CQL) { > + dytc_ignore_next_event = true; /* Ignore event */ > + err = dytc_command(DYTC_ENABLE_CQL, &output); > + if (err) > + return 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); `err` is not checked. > + if (profile != dytc_current_profile) { > + dytc_current_profile = profile; > + platform_profile_notify(); > + } > +} > +#endif > + > +static int tpacpi_dytc_init(struct ibm_init_struct *iibm) > +{ > + int err, output; > + > + 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; > @@ -9912,14 +10109,54 @@ static int tpacpi_dytc_init(struct ibm_init_struct *iibm) > if (err) > return err; > > - /* Platform supports this feature - create the group */ > - err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group); > - return err; > + /* Check DYTC is enabled and supports mode setting */ > + dytc_available = false; > + dytc_ignore_next_event = false; > + > + 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); > + dytc_available = true; > +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) > + /* Create platform_profile structure and register */ > + dytc_profile.choices = (1 << profile_low) | > + (1 << profile_balance) | > + (1 << profile_perform); > + dytc_profile.profile_get = dytc_profile_get; > + dytc_profile.profile_set = dytc_profile_set; By the way, wouldn't it be easier to initialize this struct when it's defined? ``` static platform_profile_handler dytc_profile = { .choices = ..., .profile_set = ..., .profile_get = ...., }; ``` ? > + err = platform_profile_register(&dytc_profile); > + /* > + * If for some reason platform_profiles aren't enabled > + * don't quit terminally. > + */ > + if (err) > + return 0; If I see it correctly, if `platform_profile_register()` fails for some reason, then the dytc_lapmode attribute won't be created, is that the expected behaviour? > +#endif > + /* > + * Note - this has been deprecated by the input sensor implementation, > + * but can't be removed until we confirm user space is no longer using > + */ > + dytc_lapmode_get(&dytc_lapmode); > + return device_create_file(&tpacpi_pdev->dev, &dev_attr_dytc_lapmode); Previously, the DYTC version (and the "enable bit") wasn't checked, the lap mode attribute was always created if DYTC was available. This patch changes that, why? > + } > + } > + return 0; > } > > static void dytc_exit(void) > { > - sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group); > + if (dytc_available) { > + device_remove_file(&tpacpi_pdev->dev, &dev_attr_dytc_lapmode); > +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) > + platform_profile_unregister(); `platform_profile_unregister()` is called even if `platform_profile_register()` failed. > +#endif > + dytc_available = false; > + } > } > > static struct ibm_struct dytc_driver_data = { > @@ -10103,8 +10340,15 @@ static void tpacpi_driver_event(const unsigned int hkey_event) > } > > if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) { > - dytc_lapmode_refresh(); > - lapsensor_refresh(); > + if (dytc_ignore_next_event) > + dytc_ignore_next_event = false; /*clear setting*/ Either none or all of the blocks should be surrounded with {} [1]. > + else { > + dytc_lapmode_refresh(); > +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) > + dytc_profile_refresh(); > +#endif > + lapsensor_refresh(); > + } > } > > } > -- > 2.25.1 [1]: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces Regards, Barnabás Pőcze
Hi Barnabas On 15/11/2020 13:33, Barnabás Pőcze wrote: > > 2020. november 14., szombat 16:01 keltezéssel, Mark Pearson <markpearson@lenovo.com> írta: > > Hi > > > I think there are a couple places where the BIT() macro could be used. > > >> [...] >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >> index 890dda284a00..13352ccdfdaf 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 >> @@ -9832,10 +9833,40 @@ static struct ibm_struct lcdshadow_driver_data = { >> * DYTC subdriver, for the Lenovo lapmode feature > > This comment should be updated, no? It does more than report the "lap mode" state? Agreed. > > >> */ >> >> +#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_GET 2 /* To get current IC function and mode */ >> #define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */ > > `DYTC_GET_LAPMODE_BIT` is defined a couple lines below, I think this definition > should be removed. Crud - I missed that. Amazing the compiler didn't object. Will fix. > > >> +#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 revisision */ > ^ > "revision" | ack > > >> +#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 */ > ^ > The spacing is inconsistent in the comments. | ok > > >> +#define DYTC_GET_LAPMODE_BIT 17 /* Bit 17 - lapmode. Set when on lap */ >> + >> +#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - funct setting */ > > If all letters of "function" were spelled a couple lines above, I think > it should be here as well. agreed > > >> +#define DYTC_SET_MODE_BIT 16 /* Bits 16-19 - mode setting */ >> +#define DYTC_SET_VALID_BIT 20 /* Bit 20 - 1 = on, 0 = off */ >> + > > I personally would create a DYTC_SET_COMMAND() - or similarly named - macro > along these lines: > ``` > #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) > ``` > and use that later on. I believe this helps readability and reduces the chances > of accindental mistakes. Sounds good > > >> +#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 */ > > It seems strange to me that there is a leap from 1 to 11. That one is outside my control - it's how the HW team defined the numbers :) > > >> + >> +#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 */ > > I suggest you capitalize the last two comments to be consistent with the rest > of the patch. Ack > > >> + >> +#define DYTC_DISABLE_CQL ((DYTC_MODE_BALANCE << DYTC_SET_MODE_BIT) | \ >> + (DYTC_FUNCTION_CQL << DYTC_SET_FUNCTION_BIT) | \ >> + DYTC_CMD_SET) >> +#define DYTC_ENABLE_CQL (DYTC_DISABLE_CQL | (1 << DYTC_SET_VALID_BIT)) >> [...] >> +static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode) >> +{ >> + switch (profile) { >> + case profile_low: >> + *perfmode = DYTC_MODE_QUIET; >> + break; >> + case profile_balance: >> + *perfmode = DYTC_MODE_BALANCE; >> + break; >> + case profile_perform: >> + *perfmode = DYTC_MODE_PERFORM; >> + break; >> + default: /* Unknown profile */ >> + return -EOPNOTSUPP; > > I personally think EINVAL would be better here, > just like in `convert_dytc_to_profile()`. I liked how this worked when testing. If you put in an invalid profile name then platform_profile returned EINVAL but if you got this far you'd provided a valid profile setting that this driver doesn't support and the not supported message seemed clearer. As an example 'cool' is used on HP platforms but not Lenovo. I'd like to leave this one as it is please. > > >> + } >> + return 0; >> +} >> + >> +static int dytc_perfmode_get(int *perfmode, int *funcmode) >> +{ >> + int output, err; >> + >> + if (!dytc_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; >> + err = dytc_command(DYTC_CMD_GET, &output); >> + if (err) >> + return err; > > If `DYTC_CMD_GET` fails, then CQL state is not restored. I think that may be > undesired? Agreed - thank you, that's an important catch. > > >> + /* Again ignore this event */ >> + dytc_ignore_next_event = true; >> + err = dytc_command(DYTC_ENABLE_CQL, &dummy); >> + if (err) >> + return err; >> + } >> + *perfmode = (output >> DYTC_GET_MODE_BIT) & 0xF; >> + 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 err, dytc_set; >> + int output; >> + >> + if (!dytc_available) >> + return -ENODEV; >> + >> + if (profile == 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 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; >> + } >> + dytc_set = (1 << DYTC_SET_VALID_BIT) | >> + (DYTC_FUNCTION_MMC << DYTC_SET_FUNCTION_BIT) | >> + (perfmode << DYTC_SET_MODE_BIT) | >> + DYTC_CMD_SET; >> + err = dytc_command(dytc_set, &output); >> + if (err) >> + return err; > > If I see it correctly, if CQL is turned off successfully, but the this command > fails, then the function returns with an error, but does not restore CQL state. > Which may or may not be desired? Right - not desired. I'll fix > > >> + if (cur_funcmode == DYTC_FUNCTION_CQL) { >> + dytc_ignore_next_event = true; /* Ignore event */ >> + err = dytc_command(DYTC_ENABLE_CQL, &output); >> + if (err) >> + return 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); > > `err` is not checked. ack > > >> + if (profile != dytc_current_profile) { >> + dytc_current_profile = profile; >> + platform_profile_notify(); >> + } >> +} >> +#endif >> + >> +static int tpacpi_dytc_init(struct ibm_init_struct *iibm) >> +{ >> + int err, output; >> + >> + 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; >> @@ -9912,14 +10109,54 @@ static int tpacpi_dytc_init(struct ibm_init_struct *iibm) >> if (err) >> return err; >> >> - /* Platform supports this feature - create the group */ >> - err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group); >> - return err; >> + /* Check DYTC is enabled and supports mode setting */ >> + dytc_available = false; >> + dytc_ignore_next_event = false; >> + >> + 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); >> + dytc_available = true; >> +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) >> + /* Create platform_profile structure and register */ >> + dytc_profile.choices = (1 << profile_low) | >> + (1 << profile_balance) | >> + (1 << profile_perform); >> + dytc_profile.profile_get = dytc_profile_get; >> + dytc_profile.profile_set = dytc_profile_set; > > By the way, wouldn't it be easier to initialize this struct when it's defined? > ``` > static platform_profile_handler dytc_profile = { > .choices = ..., > .profile_set = ..., > .profile_get = ...., > }; > ``` > ? Yep, it would. I'll fix. > > >> + err = platform_profile_register(&dytc_profile); >> + /* >> + * If for some reason platform_profiles aren't enabled >> + * don't quit terminally. >> + */ >> + if (err) >> + return 0; > > If I see it correctly, if `platform_profile_register()` fails for some reason, > then the dytc_lapmode attribute won't be created, is that the expected behaviour? Hmmm - that's not ideal. I'll look at this section again. > > >> +#endif >> + /* >> + * Note - this has been deprecated by the input sensor implementation, >> + * but can't be removed until we confirm user space is no longer using >> + */ >> + dytc_lapmode_get(&dytc_lapmode); >> + return device_create_file(&tpacpi_pdev->dev, &dev_attr_dytc_lapmode); > > Previously, the DYTC version (and the "enable bit") wasn't checked, the lap mode > attribute was always created if DYTC was available. This patch changes that, > why? It's an improvement/fix. It probably should have been in the original version really but I only found out after some clarification from the FW team when there were some issues on X1C6 with DYTC 4 returning lapmode always on. > > >> + } >> + } >> + return 0; >> } >> >> static void dytc_exit(void) >> { >> - sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group); >> + if (dytc_available) { >> + device_remove_file(&tpacpi_pdev->dev, &dev_attr_dytc_lapmode); >> +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) >> + platform_profile_unregister(); > > `platform_profile_unregister()` is called even if `platform_profile_register()` > failed. Agreed - I'll fix > > >> +#endif >> + dytc_available = false; >> + } >> } >> >> static struct ibm_struct dytc_driver_data = { >> @@ -10103,8 +10340,15 @@ static void tpacpi_driver_event(const unsigned int hkey_event) >> } >> >> if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) { >> - dytc_lapmode_refresh(); >> - lapsensor_refresh(); >> + if (dytc_ignore_next_event) >> + dytc_ignore_next_event = false; /*clear setting*/ > > Either none or all of the blocks should be surrounded with {} [1]. Ah - interesting. I'll fix. > > >> + else { >> + dytc_lapmode_refresh(); >> +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) >> + dytc_profile_refresh(); >> +#endif >> + lapsensor_refresh(); >> + } >> } >> >> } >> -- >> 2.25.1 > > > [1]: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces > > > Regards, > Barnabás Pőcze > Thank you! Mark
Hi 2020. november 16., hétfő 0:22 keltezéssel, Mark Pearson írta: > [...] > >> +static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode) > >> +{ > >> + switch (profile) { > >> + case profile_low: > >> + *perfmode = DYTC_MODE_QUIET; > >> + break; > >> + case profile_balance: > >> + *perfmode = DYTC_MODE_BALANCE; > >> + break; > >> + case profile_perform: > >> + *perfmode = DYTC_MODE_PERFORM; > >> + break; > >> + default: /* Unknown profile */ > >> + return -EOPNOTSUPP; > > > > I personally think EINVAL would be better here, > > just like in `convert_dytc_to_profile()`. > I liked how this worked when testing. > If you put in an invalid profile name then platform_profile returned > EINVAL but if you got this far you'd provided a valid profile setting > that this driver doesn't support and the not supported message seemed > clearer. As an example 'cool' is used on HP platforms but not Lenovo. > I'd like to leave this one as it is please. > > I have just realized that the platform profile module does not check if the profile the user wants to set is supported by the handler. As I've mentioned in my other email, I think that should be checked. You could return EOPNOTSUPP there (in the platform profile module). After reading the explanation why you want to use EOPNOTSUPP, I believe it makes sense and it's fine. > > > >> + } > >> + return 0; > >> +} > [...] Regards, Barnabás Pőcze
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 890dda284a00..13352ccdfdaf 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 @@ -9832,10 +9833,40 @@ static struct ibm_struct lcdshadow_driver_data = { * DYTC subdriver, for the Lenovo lapmode feature */ +#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_GET 2 /* To get current IC function and mode */ #define DYTC_GET_LAPMODE_BIT 17 /* Set when in lapmode */ +#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 revisision */ +#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_GET_LAPMODE_BIT 17 /* Bit 17 - lapmode. Set when on lap */ + +#define DYTC_SET_FUNCTION_BIT 12 /* Bits 12-15 - funct 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_DISABLE_CQL ((DYTC_MODE_BALANCE << DYTC_SET_MODE_BIT) | \ + (DYTC_FUNCTION_CQL << DYTC_SET_FUNCTION_BIT) | \ + DYTC_CMD_SET) +#define DYTC_ENABLE_CQL (DYTC_DISABLE_CQL | (1 << DYTC_SET_VALID_BIT)) static bool dytc_lapmode; +static bool dytc_available; +static bool dytc_ignore_next_event; static void dytc_lapmode_notify_change(void) { @@ -9889,22 +9920,188 @@ static ssize_t dytc_lapmode_show(struct device *dev, static DEVICE_ATTR_RO(dytc_lapmode); -static struct attribute *dytc_attributes[] = { - &dev_attr_dytc_lapmode.attr, - NULL, -}; +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) +static struct platform_profile_handler dytc_profile; +static enum platform_profile_option dytc_current_profile; -static const struct attribute_group dytc_attr_group = { - .attrs = dytc_attributes, -}; +static int convert_dytc_to_profile(int dytcmode, enum platform_profile_option *profile) +{ + switch (dytcmode) { + case DYTC_MODE_QUIET: + *profile = profile_low; + break; + case DYTC_MODE_BALANCE: + *profile = profile_balance; + break; + case DYTC_MODE_PERFORM: + *profile = profile_perform; + break; + default: /* Unknown mode */ + return -EINVAL; + } + return 0; +} -static int tpacpi_dytc_init(struct ibm_init_struct *iibm) +static int convert_profile_to_dytc(enum platform_profile_option profile, int *perfmode) +{ + switch (profile) { + case profile_low: + *perfmode = DYTC_MODE_QUIET; + break; + case profile_balance: + *perfmode = DYTC_MODE_BALANCE; + break; + case 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; + + if (!dytc_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; + err = dytc_command(DYTC_CMD_GET, &output); + if (err) + return err; + /* Again ignore this event */ + dytc_ignore_next_event = true; + err = dytc_command(DYTC_ENABLE_CQL, &dummy); + if (err) + return 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 err; + int funcmode, perfmode; + + err = dytc_perfmode_get(&perfmode, &funcmode); + if (err) + return err; - err = dytc_lapmode_get(&dytc_lapmode); - /* If support isn't available (ENODEV) then don't return an error - * but just don't create the sysfs group + /* 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 err, dytc_set; + int output; + + if (!dytc_available) + return -ENODEV; + + if (profile == 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 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; + } + dytc_set = (1 << DYTC_SET_VALID_BIT) | + (DYTC_FUNCTION_MMC << DYTC_SET_FUNCTION_BIT) | + (perfmode << DYTC_SET_MODE_BIT) | + DYTC_CMD_SET; + err = dytc_command(dytc_set, &output); + if (err) + return err; + if (cur_funcmode == DYTC_FUNCTION_CQL) { + dytc_ignore_next_event = true; /* Ignore event */ + err = dytc_command(DYTC_ENABLE_CQL, &output); + if (err) + return 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(); + } +} +#endif + +static int tpacpi_dytc_init(struct ibm_init_struct *iibm) +{ + int err, output; + + 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; @@ -9912,14 +10109,54 @@ static int tpacpi_dytc_init(struct ibm_init_struct *iibm) if (err) return err; - /* Platform supports this feature - create the group */ - err = sysfs_create_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group); - return err; + /* Check DYTC is enabled and supports mode setting */ + dytc_available = false; + dytc_ignore_next_event = false; + + 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); + dytc_available = true; +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) + /* Create platform_profile structure and register */ + dytc_profile.choices = (1 << profile_low) | + (1 << profile_balance) | + (1 << profile_perform); + dytc_profile.profile_get = dytc_profile_get; + dytc_profile.profile_set = dytc_profile_set; + err = platform_profile_register(&dytc_profile); + /* + * If for some reason platform_profiles aren't enabled + * don't quit terminally. + */ + if (err) + return 0; +#endif + /* + * Note - this has been deprecated by the input sensor implementation, + * but can't be removed until we confirm user space is no longer using + */ + dytc_lapmode_get(&dytc_lapmode); + return device_create_file(&tpacpi_pdev->dev, &dev_attr_dytc_lapmode); + } + } + return 0; } static void dytc_exit(void) { - sysfs_remove_group(&tpacpi_pdev->dev.kobj, &dytc_attr_group); + if (dytc_available) { + device_remove_file(&tpacpi_pdev->dev, &dev_attr_dytc_lapmode); +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) + platform_profile_unregister(); +#endif + dytc_available = false; + } } static struct ibm_struct dytc_driver_data = { @@ -10103,8 +10340,15 @@ static void tpacpi_driver_event(const unsigned int hkey_event) } if (hkey_event == TP_HKEY_EV_THM_CSM_COMPLETED) { - dytc_lapmode_refresh(); - lapsensor_refresh(); + if (dytc_ignore_next_event) + dytc_ignore_next_event = false; /*clear setting*/ + else { + dytc_lapmode_refresh(); +#if IS_ENABLED(CONFIG_ACPI_PLATFORM_PROFILE) + dytc_profile_refresh(); +#endif + lapsensor_refresh(); + } } }
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 drivers/platform/x86/thinkpad_acpi.c | 278 +++++++++++++++++++++++++-- 1 file changed, 261 insertions(+), 17 deletions(-)