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

[Werner Sembach <wse@tuxedocomputers.com>]

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.

>
>> 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.
>
>>
>> Thanks,
>> Armin Wolf
>
> Best regards,
>
> Werner
>