On Mon, 18 Nov 2024, Joshua Grisham wrote: > 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! > > 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 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. > > 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? > > 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 ? Hi Joshua, There are some pre-existing 'please report to' prints already. For the different levels of information needed for production vs troubleshooting, I sugges you rely mostly on dynamic debugging though so if you need to have a deeper look, just ask the reporter to enable dyndbg for the driver so you don't need to have them spamming logs in normal situations. There's nothing wrong with having github as bug reporting place as long as somebody keeps an eye on the reports, obviously. You can add a MAINTAINERS entry with B:. I also suggest if you really plan to keep an eye on this driver, add yourself as the maintainer too as then tools would automatically add you to the relevant patches so you can comment/review. I can still manage the merging the patches into pdx86 repo and PRs to Linus so you don't need to worry about handling such a boring part of maintainership :-). > 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. It would be unnecessary dead code. > 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. That would be more Hans' domain of expertize so I suggest you ping him with a private mail as he has decided to pay less attention on pdx86 mailing list's posts: https://lore.kernel.org/platform-driver-x86/02b4c051-ea11-4fe9-876d-070d18cd84bf@xxxxxxxxxx/ > 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. Try to replace as many literals with named defines, it always helps the code reader. Especially, if you end up adding a comment to explain a literal, you really would want to use a named define instead and forgo the comment as the code explains itself. There are some indentation/alignment/style issues, e.g., braces around "else" and on continuation lines. Be silent when no error occurs (=no non-dbg level prints). Remove any double empty lines. Don't split print strings to multiple lines to allow easier grep. Please do realize it's much easier to pinpoint review comments when you post actual patch on the list so I hope you won't shy out too far from that. We do appreciate contributions and try to help you out to get them into shape for merging. -- i.
