2020. november 14., szombat 16:01 keltezéssel, Mark Pearson <markpearson@xxxxxxxxxx> í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
