Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor driver to use existing platform device instead of creating new one #44

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

joshuagrisham
Copy link
Owner

As part of an ongoing dialog with the Platform x86 maintainers, I have now refactored the driver to remove the extra "hard coded" platform device samsung-galaxybook, and instead this driver will use the platform device which is already already created from the ACPI device.

Here is a link to the mailing list thread with more background: https://lore.kernel.org/platform-driver-x86/CAMF+KeaSarRT3weYhiCFO=Th5ZWMf=nvi53A+ggKYq2wBYAJpw@mail.gmail.com/T/#ma17bd2db196e1b6872e14963b84d1117bdb0f67d

As the hard-coded device has now been removed, and the sysfs attribute paths will now be "dynamic" based on which ACPI Device ID the user's device has, I have included some instructions in the README on how to now handle this, as well as an example udev rules file which will create a symlink to the device under the fixed path /dev/samsung-galaxybook no matter which of the supported ACPI Device IDs the user's device has.

I have tested this extensively on my own device, and everything does appear to still be working as before. One thing that I would be especially interested in is if @seaasses you might be willing and able to test this branch (https://github.com/joshuagrisham/samsung-galaxybook-extras/tree/refactor-use-existing-platform-device) and see if your issues from before (especially #32 ) are still ok with this latest branch?

If it is looking good then I think I am very close to being ready to submit a patch for this to mainline. There are a few small feedbacks I am waiting for but hopefully nothing more major... this PR is getting us closer, in any case!

@mhagnumdw
Copy link

@joshuagrisham you are awesome! I can also test on my device (Galaxy Book3 Ultra NP960XFH-XA1BR). Just to confirm: the installation process remains unchanged?

@joshuagrisham
Copy link
Owner Author

Yes please do @mhagnumdw , it would be much appreciated!!

Yes, the installation process should still be basically exactly the same -- feel free to use some of the examples in the README of this branch otherwise to be 100% sure.

Do note that if you have anything outside of the driver you are using to interact with various features (scripts etc) that rely on the path /sys/devices/platform/samsung-galaxybook/..., these may need to be adjusted (again more details in the latest README of this branch)!

@rapgenic
Copy link
Contributor

rapgenic commented Dec 8, 2024

Hi @joshuagrisham, sorry not being able to test your changes recently (I was busy with graduation :)

I have built this branch and for now everything is working with my Galaxy Book 360!

I will daily drive it for a while, reporting if anything is not working with my device.

intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this pull request Dec 9, 2024
This patch will add a new driver for Samsung Galaxy Book series notebook
devices. This should hopefully include all suggestions from my original
mailing list feedback thread [1], as well as a first version for
associated updates to the documentation, Kconfig, Makefile, and the
MAINTAINERS file related to this new driver.

I have tested the driver both using m and y in the config, as well as
with various other options mentioned in the patch checklist of the
documentation.

Other users with other device IDs have also tested successfully using
a version of these same driver updates which I have maintained in a
separate branch of my GitHub repository [2].

I have made an attempt with the coding style to find a balance between what
is written in the kernel documentation and what actually exists in current
x86 platform drivers, but any feedback on this (or anything else) is
certainly welcome!

[1]: https://lore.kernel.org/platform-driver-x86/CAMF+KeYus9dW00WNJMLVxLLHdG9JgCfrGJ491fu7NM8GAEqqCg@mail.gmail.com/
[2]: joshuagrisham/samsung-galaxybook-extras#44

Signed-off-by: Joshua Grisham <josh@joshuagrisham.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants