-
Notifications
You must be signed in to change notification settings - Fork 16
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
add installation with dkms #31
base: main
Are you sure you want to change the base?
Conversation
Tested this on an Acer Swift XSFX14-41G running Fedora 38. Module continued to work and was persistent through a kernel update. For Fedora, you'd need to install the following at a minimum: |
Tested on Acer Aspire 7 A715-42G and Ubuntu 23.04. It seems work flawless. |
Working on Acer Nitro AN515-57, thank you for this PR! |
The PR repo doesn't accept issues, so putting it here. I tried uninstalling and installing it again and got this (in Pop!_OS): |
Just remembered to update on comments above. I fixed it by adding a systemd service to force health_mode to be 1 on startup. I have no experience with building drivers but maybe to fix this it would be needed to persist this value in a file and get that file on reboot/kernel updates. |
I do not know how the driver actually works, so I can't help you there, but there indeed was a problem with the uninstall script. The script is now fixed, pull the changes and use the new one if you want a cleaner uninstallation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The PR looks good to me and I would like to merge it after the review changes have been addressed.
1) Install DKMS and generic kernel headers (this will always get you the latest headers), on Debian or Ubuntu it can be done with: | ||
|
||
``` | ||
sudo apt install dkms linux-headers-generic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked what linux-headers-generic would install on my system and you are right, it depends on the package for the latest kernel headers. But what if you are not running the latest kernel version - as I am. Since the build instructions already describe how to install the headers for the current kernel, I would remove the "linux-headers-generic" from this apt invocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this file and the other dkms related files to a new directory contrib/dkms, move your instructions to contrib/dkms/README.md and link to this file from README.md?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some further remarks.
@@ -2,7 +2,7 @@ obj-m += acer-wmi-battery.o | |||
PWD := $(CURDIR) | |||
|
|||
all: | |||
make -C /lib/modules/$(shell uname -r)/build M=$(PWD) modules | |||
make -C /lib/modules/$(KERNELVERSION)/build M=$(PWD) modules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need to be able to pass a kernel version to the Makefile, it would be better to make this optional, i.e. specify a default value for KERNELVERSION
. This can be accomplished by defining the variable at the top of the Makefile like this:
KERNELVERSION ?= "$(shell uname -r)"
.
sudo ./install.sh | ||
``` | ||
|
||
The driver will now automatically load at boot and be recompiled after a kernel upgrade. Reboot to use it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reboot to use it.
modprobe
should also work?
@@ -19,13 +19,13 @@ Any feedback on how it works on other Acer laptops would be appreciated. | |||
## Building | |||
|
|||
Make sure that you have the kernel headers for your kernel installed | |||
and type `make` in the cloned project directory. In more detail, | |||
and type `make KERNELVERSION=$(uname -r)` in the cloned project directory. In more detail, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build instructions should not need to change once you adapt the Makefile as described above.
Added a persistent installation script, that does 2 things - installs the module into the kernel with DKMS (and makes it survive kernel upgrades) and sets it to load at boot.
Added an uninstallation script, that reverts these actions.
Tested on Ubuntu 23.04