Message-ID: <2273f725-aab4-47a6-bccf-22c1789f5e02@tuxedocomputers.com>
Date: Mon, 22 Sep 2025 18:03:15 +0200
From: Werner Sembach <wse@...edocomputers.com>
To: Armin Wolf <W_Armin@....de>, ilpo.jarvinen@...ux.intel.com,
 hdegoede@...hat.com, chumuzero@...il.com, corbet@....net, cs@...edo.de,
 ggo@...edocomputers.com
Cc: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 platform-driver-x86@...r.kernel.org, rdunlap@...radead.org,
 alok.a.tiwari@...cle.com, linux-leds@...r.kernel.org, lee@...nel.org,
 pobrn@...tonmail.com
Subject: Re: [PATCH v3 1/2] platform/x86: Add Uniwill laptop driver


Am 18.09.25 um 23:25 schrieb Armin Wolf:
> 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.
ack
>
> 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?
ofc, but only the hwmon and keyboard part apply to our devices
>
>>>
>>>> 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.

ack

working on my patch series here: 
https://gitlab.com/tuxedocomputers/development/packages/linux/-/tree/tuxedo-drivers_upstream_wip 
will send it separately after this got merged

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

Powered by blists - more mailing lists