Re: [PATCH v3 1/2] platform/x86: Add Uniwill laptop driver

[Armin Wolf <W_Armin@gmx.de>]

Am 09.09.25 um 21:13 schrieb Werner Sembach:

>
> Am 09.09.25 um 11:18 schrieb Werner Sembach:
>> Hi,
>>
>> Am 08.09.25 um 18:29 schrieb Armin Wolf:
>>>>> +static ssize_t fn_lock_show(struct device *dev, struct 
>>>>> device_attribute *attr, char *buf)
>>>>> +{
>>>>> +    struct uniwill_data *data = dev_get_drvdata(dev);
>>>>> +    unsigned int value;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = regmap_read(data->regmap, EC_ADDR_BIOS_OEM, &value);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    return sysfs_emit(buf, "%s\n", str_enable_disable(value & 
>>>>> FN_LOCK_STATUS));
>>>>> +}
>>>>> +
>>>>> +static DEVICE_ATTR_RW(fn_lock);
>>>>
>>>> The fn_lock register value does not automatically get updated by 
>>>> pressing the fn+esc key (unlicke the super_key_lock), so the driver 
>>>> needs to do that manually.
>>>>
>>>> Another posibility is: uniwill sometimes have a "config" and an 
>>>> "immediate" value for a setting, waybe we have the config value 
>>>> here (and have the immediate value for the super_key_lock)
>>>>
>>>> Also I realized: The value here is preserved on hot, but not on 
>>>> cold reboots, maybe this should be initialized by the driver for 
>>>> consistency?
>>>>
>>> fn_lock should not change when the users presses Fn + ESC, instead 
>>> this setting controls whether the EC will enter Fn lock mode when 
>>> the user presses
>>> this key combination.
>>
>> At least on my device Fn + ESC does toggle the Fn lock regardless of 
>> this setting. How I love these Uniwill inconsistencies ...
>>
>> I talked with Christoffer and he said that the "Intel Project" line 
>> from Uniwill does behave differently at multiple locations
>>
>> If the devices really behave differently we have the first mutually 
>> exclusive feature here: FN Lock Enable vs FN Lock Toggle
>
> Thinking about how to name this to make it consistent and clear by 
> name only what is happening, my idea would be:
>
> - fn_lock_toggle_enable (for the behavior on your device)
>
> - fn_lock_enable (for the behavior on my devices)
>
> - super_key_toggle_enable (for the behavior on your device)
>
> - super_key_enable (for the behavior on my devices)
>
> - touchpad_toggle_enable (for the behavior on your device)
>
> There is no touchpad_enable as this is handled by userspace.
>
OK, i will rename the sysfs attributes accordingly. However i suggest that support for the other sysfs attributes
be added in a separate patch series, as i want to get this one merged as soon as possible.

Could you test the next revision of this patch series on your device as  the  other testers sometimes take a lot of time to respond?

>>
>>> Additionally, some models seem to allow users to change those 
>>> settings inside the BIOS itself, so i am against overwriting the
>>> boot configuration when loading the driver.
>> That's probably what's sets the value on cold boot.
>>>>> +static ssize_t super_key_lock_show(struct device *dev, struct 
>>>>> device_attribute *attr, char *buf)
>>>>> +{
>>>>> +    struct uniwill_data *data = dev_get_drvdata(dev);
>>>>> +    unsigned int value;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = regmap_read(data->regmap, EC_ADDR_SWITCH_STATUS, &value);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    return sysfs_emit(buf, "%s\n", str_enable_disable(!(value & 
>>>>> SUPER_KEY_LOCK_STATUS)));
>>>>> +}
>>>>> +
>>>>> +static DEVICE_ATTR_RW(super_key_lock);
>>>>
>>>> I did not know what "super_key_lock" was supposed to mean at first, 
>>>> a more fitting name would be super_key_enable imho.
>>>>
>>>> Cold vs hot reboot volatility not tested, but wouldn't hurt to 
>>>> initialize imho as i don't trust uniwill to be consistent in this 
>>>> point across multiple device generations.
>>>>
>>> This sysfs attribute controls whether or not the super key can be 
>>> locked using a key combination i forgot about. Initializing those 
>>> settings
>>> is something best done by userspace, i suggest to use a udev rule 
>>> for that.
>>
>> No again, at least on the devices i have here: the key combination is 
>> fn+f9, but not present on all devides (the fn functions get shifted 
>> quite around on different uniwill devices anyway)
>>
>> The combination still works when this is set to disable and just sets 
>> it to enable.
>>
>>>
>>>>> +
>>>>> +static ssize_t touchpad_toggle_store(struct device *dev, struct 
>>>>> device_attribute *attr,
>>>>> +                     const char *buf, size_t count)
>>>>> +{
>>>>> +    struct uniwill_data *data = dev_get_drvdata(dev);
>>>>> +    unsigned int value;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = sysfs_match_string(uniwill_enable_disable_strings, buf);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    if (ret)
>>>>> +        value = 0;
>>>>> +    else
>>>>> +        value = TOUCHPAD_TOGGLE_OFF;
>>>>> +
>>>>> +    ret = regmap_update_bits(data->regmap, EC_ADDR_OEM_4, 
>>>>> TOUCHPAD_TOGGLE_OFF, value);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    return count;
>>>>> +}
>>>>> +
>>>>> +static ssize_t touchpad_toggle_show(struct device *dev, struct 
>>>>> device_attribute *attr, char *buf)
>>>>> +{
>>>>> +    struct uniwill_data *data = dev_get_drvdata(dev);
>>>>> +    unsigned int value;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = regmap_read(data->regmap, EC_ADDR_OEM_4, &value);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    return sysfs_emit(buf, "%s\n", str_enable_disable(!(value & 
>>>>> TOUCHPAD_TOGGLE_OFF)));
>>>>> +}
>>>>> +
>>>>> +static DEVICE_ATTR_RW(touchpad_toggle);
>>>> What exactly does this do? Seems like a noop on my testing devices. 
>>>> Also is touchpad disable not already handled by userspace?
>>>
>>> This settings controls whether or not the user can disable the 
>>> internal touchpad using a specific key combination.
>>
>> Ok, this function seems to be not present on non Intel project 
>> devices from Uniwill. Here the touchpad toggle just sends a key 
>> combination (Super + Control + KEY_ZENKAKUHANKAKU or F24 depending on 
>> kernel version) and lets userspace handle the rest.
>>
>> Never mind then.
>>
>>>>> +static const struct hwmon_ops uniwill_ops = {
>>>>> +    .visible = 0444,
>>>>> +    .read = uniwill_read,
>>>>> +    .read_string = uniwill_read_string,
>>>>> +};
>>>>
>>>> .visible should hide gpu temp sensor on devices that don't have a 
>>>> dgpu and therefore not gpu temp sensor (the value is stuck at 0 on 
>>>> these devices)
>>>>
>>>> also the number of fan might also not always be exactly 2
>>>>
>>> I see, i will introduce separate feature flags for each sensor.
>> thanks
>>>>> +static int __init uniwill_init(void)
>>>>> +{
>>>>> +    const struct dmi_system_id *id;
>>>>> +    int ret;
>>>>> +
>>>>> +    id = dmi_first_match(uniwill_dmi_table);
>>>>> +    if (!id) {
>>>>> +        if (!force)
>>>>> +            return -ENODEV;
>>>>> +
>>>>> +        /* Assume that the device supports all features */
>>>>> +        supported_features = UINT_MAX;
>>>>
>>>> in the future there might be mutually exclusive feature (for 
>>>> example when Uniwil repurposes EC registers)
>>>>
>>>> my suggestion would be to have a "force_supported_features" in 
>>>> addition that overwrites the supported_features list (also for 
>>>> devices that are in the list)
>>>>
>>>> so something like:
>>>>
>>>> if (!id && !force)
>>>>
>>>>     return -ENODEV
>>>>
>>>> if (force)
>>>>
>>>>     supported_features = force_supported_features
>>>>
>>>> else
>>>>
>>>>     supported_features = (uintptr_t)id->driver_data;
>>>>
>>> Interesting idea, but i would prefer to keep the individual feature 
>>> bit definitions private. Because of this i suggest we
>>> look into this idea once we actually encounter such a situation 
>>> where we have conflicting feature bits.
>>
>> Then maybe just have all the features as separate module parameters?
>>
>> On this note: Maybe also do the FN Key handling based on a feature 
>> bit? Not that i see a particular reason why you wouldn't want to have 
>> it, but for consistency and debugging reasons (and also if sometimes 
>> ins the future an incompatibility arises here because Uniwill 
>> repurposed a wmi event or something).
>>
>> Just thinking out loud.
>>
I suggest that we implement the handling around those additional feature bits inside a separate patch series.

Thanks,
Armin Wolf

>>>
>>> Thanks,
>>> Armin Wolf
>>
>> Best regards,
>>
>> Werner
>>