Re: [External] Re: [PATCH v2 3/3] platform/x86: thinkpad_acpi: Add platform profile support

[Mark Pearson <markpearson@lenovo.com>]

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