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
