Am 18.11.24 um 14:51 schrieb Joshua Grisham:
Hello! I have created a platform driver for Samsung Galaxy Book series notebooks which has now gone through several iterations and contributions from several other community members. Based on stars and community involvement I would guess that the usage of the driver is more than at least 100 users (if not more?) across multiple different generations of devices and many different distributions, so hopefully we have ironed out a lot of issues by now!
Nice work! Improving the hardware support for notebooks under linux is always welcomed.
The existing driver samsung-laptop is of course somewhat famous/notorious for how it works, but on newer devices (primarily Samsung Galaxy Book series devices but does include a few others), Samsung is using a new ACPI device called "SCAI" which is what this new driver is built on, and the functionality is totally different. There are only a few ACPI methods on this device that then actually control a lot of different features; the "magic" is in building various payloads to steer all of these different functionalities even though it is often using the same ACPI method. It is my opinion that, as we now have achieved some level of stability with this driver, it would be good to try and get it added to mainline as having it in mainline will add a lot of benefits (even larger number of users who will gain benefit from this, better quality and standardization with involvement from maintainers and the larger community, etc).
I support your initiative to upstream your driver. Having such a piece of software upstream helps everyone.
I have myself tried to adhere to many of the existing patterns that exist within other pdx86 drivers and the community has helped to find and ensure (and in some cases even directly contributed to that) various features are using standard interfaces such as with the battery extension, platform profile, etc, in a way that seems to be unified with existing platform drivers as well. The driver code is currently located here: https://github.com/joshuagrisham/samsung-galaxybook-extras/blob/main/samsung-galaxybook.c As there are a few variants of what features are supported on different devices (even devices with the same ACPI device id) then one of the key principles that I have tried to now follow with the driver is that each feature tries to check that it works or not (receives an error code in the payload from the ACPI method) before "enabling" the feature (creating a sysfs attribute or registering a new device etc) when the module is probed and loaded.
Sounds like a good strategy to me, being able to automatically detect which features are available is usually better than having a very long quirk list.
Instead of just sending the code as-is in a new patch then I thought to ask you all as the PDX86 maintainers if there is anything glaring that you would prefer should be changed or re-designed before we try to push this in as a patch and add this driver to the kernel?
After looking at the driver, i would advise you to drop the acpi_driver stuff and instead implement the whole driver as a platform_driver. Does the kernel already create a suitable platform device with the name "SAM04[number]:[number]"?
You can see more background and what features are supported in the README file here: https://github.com/joshuagrisham/samsung-galaxybook-extras/blob/main/README.md A few potentially "controversial" bits that I can highlight already now: 1. various failure messages or "unsupported features" write a warning that directs users to create an issue in my own Github repository instead of in Bugzilla -- maybe this is ok at the beginning but assume it would be better to just remove some of this info from the message and/or direct users to create a new bug in Bugzilla under the right component there ?
As a general rule driver should be quiet if everything works, so unsupported features should not result in a warning message. The other error messages should just contain the message without any bugzilla/github links since stable kernel users might want to use the bugtrackers of their distro first.
2. some features where Kernel version are checked for handling some things different for older versions of the kernel, but all of this I would take away before submitting a patch
Yes, please remove any kernel version checks. Thanks, Armin Wolf
3. usage of the i8042 filter and ACPI hotkey notifications to handle a few of the hotkey actions within the driver itself instead of just emitting input events and allow userspace to handle the actions (namely cycling through keyboard backlight levels, performance modes, etc) This last item (executing hotkey actions in kernel space) is not totally unprecedented either, as I have seen there seems to exist similar i8042 filters driving hotkey actions in msi-laptop, toshiba_acpi, and dell-laptop and ACPI notifications from hotkeys driving actions in several x86 platform drivers as well (dell-laptop, acer-wmi, asus-laptop, ideapad-laptop, etc; this is an even more common pattern than using an i8042 filter, it seems). The problem with just emitting the "right" input events and relying on the userspace to handle this stuff in the right way is that 1) there are not really keycodes that exist for exactly the keys we want here (even though "Keyboard Backlight Cycle" and some kind of "Performance Mode" hotkeys are very common on laptops today) and 2) functionality for how to handle these kind of events do not really support these use-cases either (an example if you read through the discussion here: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/41 and especially several of the comments from Benjamin Berg, that implementation of "Keyboard Backlight Toggle" is just on vs off and does not cycle, and there should either be special handling of this or a new key is created specifically for this purpose... this was from 5-6 years ago and the state of this has not changed since then from what I can tell). It is because of these same problems that I assume the existing PDX86 drivers do in fact implement some of this hotkey action logic in the kernel space, in a similar way that I have tried to do in this new samsung-galaxybook driver. I am not sure the appetite for having even more of this pattern exist and/or if there are any details of the implementation that you all would wish that I should tweak a bit? I am very open to any kind of feedback on this. Any other discussion or questions are of course welcome! Otherwise and/or once things are to a point that is looking good then I can create and submit a patch for this new driver. Thank you! Best regards, Joshua Grisham
