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

[Werner Sembach <wse@tuxedocomputers.com>]

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

> 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