Adding a new platform driver samsung-galaxybook

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 ?

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

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




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux