|
From: Mark P. <mar...@le...> - 2020-07-22 19:29:47
|
Hi Mario
On 7/22/2020 2:46 PM, Limonciello, Mario wrote:
<snip>
>>
>> +DYTC Thermal mode status and control
>> +------------------------------------
>> +
>> +sysfs: dytc_perfmode
>> +
>> +Lenovo Thinkpad platforms with DYTC version 5 and newer have enhanced
>> firmware to
>> +provide improved performance control.
>> +
>> +The firmware can be controlled by hotkeys (FN+H, FN+M, FN+L) to switch the
>> +operating mode between three different modes. This sysfs node provide a
>> better
>> +interface for user space to use
>
> So is userspace also notified in some way when you use the hotkey to change, or
> is the event usurped by the EC? Is this by the event TP_HKEY_EV_THM_CSM_COMPLETED?
>
I haven't added that yet - my aim with this patch was to get the sysfs
API available. I'll look at adding the notification.
> You might consider to mention what other interfaces will conflict with this
> and document them and/or artificially block them when this is loaded to prevent
> such a conflict.
I'm afraid I don't know what other interface will be conflicted with. Is
there anything in particular I should be looking for? What did you have
in mind?
The firmware is operating by default and this patch is just providing
user space with a way of determining the current mode and changing it by
an alternate mechanism than hotkeys (I know some people dislike the
hotkeys...)
>
<snip>
>> +
>> +The sysfs entry provides the ability to return the current status and to set
>> the
>> +desired mode. For example::
>> +
>> + echo H > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
>> + echo M > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
>> + echo L > /sys/devices/platform/thinkpad_acpi/dytc_perfmode
>
> Doesn't this need ABI documentation submitted as part of the patch?
OK - I'll need some help here as I'm not sure what I missed. Isn't that
what this part of the patch is covering? If you can give me some
pointers for what I should be putting where I'll do that.
>
<snip>
>> +
>> + if (perfmode == DYTC_MODE_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 {
>> + /* 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*/
>> + err = dytc_command(0x000F1001 /*Disable CQL*/, &output);
>
> Why not put 0x000F1001 and 0x001F1001 as defines at the top?
Fair point - I will fix that.
>
<snip>
>> +
>> + switch (perfmode) {
>> + case DYTC_MODE_PERFORM:
>> + /* High performance is only available in deskmode */
>> + if (funcmode == DYTC_FUNCTION_CQL)
>> + return snprintf(buf, PAGE_SIZE, "Medium (Reduced as lapmode
>> active)\n");
>> + else
>> + return snprintf(buf, PAGE_SIZE, "High\n");
>> + case DYTC_MODE_QUIET:
>> + return snprintf(buf, PAGE_SIZE, "Low\n");
>> + case DYTC_MODE_BALANCE:
>> + return snprintf(buf, PAGE_SIZE, "Medium\n");
>> + default:
>> + return snprintf(buf, PAGE_SIZE, "Unknown (%d)\n", perfmode);
>> + }
>> +}
>
> I think it's pretty confusing that you write "H/M/L" into this file, but you
> get back a full string.
The main reason here for the string is the need to let the user know
they are operating in medium mode even though high has been configured -
because the device is on their lap.
My thinking was you can parse the first letter to get H/M/L but more
information is available for the subtleties.
I considered another letter but couldn't determine something that was
obvious to a user (Lower case 'h' is my best candidate?) and decided a
string was nicer.
I'd appreciate input from others as to the best thing to provide here.
>
>> +
>> +static ssize_t dytc_perfmode_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + int err;
>> +
>> + switch (buf[0]) {
>> + case 'l':
>> + case 'L':
>> + err = dytc_perfmode_set(DYTC_MODE_QUIET);
>> + break;
>> + case 'm':
>> + case 'M':
>> + err = dytc_perfmode_set(DYTC_MODE_BALANCE);
>> + break;
>> + case 'h':
>> + case 'H':
>> + err = dytc_perfmode_set(DYTC_MODE_PERFORM);
>> + break;
>> + default:
>> + err = -EINVAL;
>> + pr_err("Unknown operating mode. Ignoring\n");
>
> Shouldn't this be dev_err?
Ack - I will correct
<snip>
>>
>> + /* Check DYTC is enabled and supports mode setting */
>> + dytc_mode_available = 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) {
>> + pr_info("DYTC thermal mode configuration available\n");
>
> I would argue this isn't useful to most people.
> 1) You should decrease this to debug for use with dynamic debugging
> 2) Output in the log what integer value you returned back in case of a need
> to identify future firmware bugs.
Agreed on both fronts. I will fix.
>
>> + dytc_mode_available = true;
>
> I think you shouldn't set this flag until after the group is actually created.
>
Agreed. I will fix
Thanks for the feedback - very much appreciated.
Mark
|
