On 7/29/20 2:32 PM, Barnabás Pőcze wrote:
External email: Use caution opening links or attachments
2020. július 29., szerda 19:11 keltezéssel, Daniel Dadap <ddadap@xxxxxxxxxx> írta:
[...]
I think
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
(or similar)
should be defined before any includes since you use pr_*().
Otherwise the messages won't be prefixed by the module name.
Yes, sloppy of me to not add that when adding the pr_err() statements,
which weren't in the v1 patch.
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/pci.h>
+#include <linux/vga_switcheroo.h>
+#include <linux/delay.h>
+
[...]
+static void set_mux_state(acpi_handle handle, enum mux_state_command command)
+{
+ acpi_helper(handle, MXDS, command);
+}
+
It's really a nitpick and a moot point, but I'd think set_mux_state() takes the state as
one of its arguments, not the command to set that state.
Yeah, the old function signature from the v1 patch was more intuitive. I
was just trying to save a switch statement since the caller already had
one to convert the switcheroo IDs to the appropriate commands, but
there's really no point in doing so if it makes the code less clear.
[...]
+static int __init mxds_gmux_init(void)
+{
+ int ret = 0;
+ struct pci_dev *dev = NULL;
+
+ while ((dev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) {
+ switch (dev->vendor) {
+ case 0x8086:
+ pci_dev_put(ig_dev);
+ ig_dev = pci_dev_get(dev);
+ break;
+ case 0x10de:
+ pci_dev_put(dg_dev);
+ dg_dev = pci_dev_get(dev);
+ break;
+ default:
+ break;
+ }
+ }
+
I think I mentioned it last time, I personally dislike magic numbers,
so I'd use PCI_VENDOR_ID_{INTEL,NVIDIA} from <linux/pci_ids.h> here, but
I guess it's personal preference, so I don't want to bother you with that
anymore. If you use magic numbers, then I think comments would be much
appreciated.
You did mention that, and I intended to address it, but forgot to. Thank
you for the reminder.
[...]
+static void __exit mxds_gmux_exit(void)
+{
+ if (vga_switcheroo_registered)
+ vga_switcheroo_unregister_handler();
The exit method of a module will not be run if it fails to load,
so I think "vga_switcheroo_registered" is not necessary.
Good point. This began life as part of a multi-purpose kernel module,
where it was possible for the module as a whole to load even if the
switcheroo part failed.
+ pci_dev_put(ig_dev);
+ pci_dev_put(dg_dev);
+}
+module_exit(mxds_gmux_exit);
--
2.18.4
Barnabás Pőcze
