On Wed, Sep 2, 2020 at 8:37 PM Daniel Dadap <ddadap@xxxxxxxxxx> wrote: > > Some upcoming notebook designs utilize display muxes driven by a > pair of ACPI methods, MXDM to query and configure the operational > mode of the mux (integrated only, discrete only, hybrid non-muxed, > hybrid with dynamic mux switching), and MXDS to query and set the > mux state when running in dynamic switch mode. > > Add a vga-switcheroo driver to support switching the mux on systems > with the ACPI MXDM/MXDS interface. The mux mode cannot be changed > dynamically (calling MXDM to change the mode won't have effect until > the next boot, and calling MXDM to read the mux mode returns the > active mode, not the mode that will be enabled on next boot), and > MXDS only works when the mux mode is set to dynamic switch, so this > driver will fail to load when MXDM reports any non-dynamic mode. > > This driver currently only supports systems with Intel integrated > graphics and NVIDIA discrete graphics. It will need to be updated if > designs are developed using the same interfaces which utilize GPUs > from other vendors. Thanks for an update. My comments below. > v2,v3: misc. fixes suggested by Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx> > v4: misc. changes suggested by Lukas Wunner <lukas@xxxxxxxxx> This should go after the cutter '---' line below. > Signed-off-by: Daniel Dadap <ddadap@xxxxxxxxxx> > --- > MAINTAINERS | 6 + > drivers/platform/x86/Kconfig | 9 ++ > drivers/platform/x86/Makefile | 2 + > drivers/platform/x86/mxds-gmux.c | 265 +++++++++++++++++++++++++++++++ ... > +config MXDS_GMUX > + tristate "ACPI MXDS Gmux Driver" > + depends on ACPI_WMI > + depends on ACPI Is not this implied by the above? > + depends on VGA_SWITCHEROO > + help > + This driver provides support for ACPI-driven gmux devices which are > + present on some notebook designs with hybrid graphics. The stuff in Kconfig and Makefile is grouped and sorted by alphabet in each group. Please, follow. ... > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * mxds-gmux: vga_switcheroo mux handler for ACPI MXDS muxes Please, remove the file name from the file. > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see <http://www.gnu.org/licenses>. > + * Above should be removed since we use SPDX. > + */ ... > +#include <linux/module.h> > +#include <linux/acpi.h> > +#include <linux/pci.h> > +#include <linux/vga_switcheroo.h> > +#include <linux/delay.h> Sorted is easier to read. ... > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("vga_switcheroo mux handler for ACPI MXDS muxes"); > +MODULE_AUTHOR("Daniel Dadap <ddadap@xxxxxxxxxx>"); Usually we put this at the end of the file. > +/* > + * The mux doesn't have its own ACPI HID/CID, or WMI wrapper, so key off of > + * the WMI wrapper for the related WMAA method for backlight control. > + */ > +MODULE_ALIAS("wmi:603E9613-EF25-4338-A3D0-C46177516DB7"); But this one depends. ... > +static struct pci_dev *ig_dev, *dg_dev; > +static acpi_handle internal_mux_handle; > +static acpi_handle external_mux_handle; Global variables?! Please get rid of them. ... > +enum acpi_method { > + MXDM = 0, > + MXDS, > + NUM_ACPI_METHODS > +}; Hmm... any real need for this enum? ... > +enum mux_state_command { > + MUX_STATE_GET = 0, > + MUX_STATE_SET_IGPU = BIT(0), > + MUX_STATE_SET_DGPU = BIT(4) | BIT(0), > +}; #include <linux/bits.h> ... > +static acpi_integer acpi_helper(acpi_handle handle, enum acpi_method method, > + acpi_integer action) > +{ > + union acpi_object arg = { > + .integer = { .type = ACPI_TYPE_INTEGER, .value = action } > + }; > + struct acpi_object_list in = {.count = 1, .pointer = &arg}; Be consistent with spaces surrounding the structure content. > + acpi_integer ret; > + acpi_status status; > + > + status = acpi_evaluate_integer(handle, acpi_methods[method], &in, &ret); > + Redundant blank line. > + if (ACPI_FAILURE(status)) { > + pr_err("ACPI %s failed: %s\n", acpi_methods[method], > + acpi_format_exception(status)); Why not acpi_handle_err() ? > + return 0; > + } > + > + return ret; > +} ... > +static enum vga_switcheroo_client_id mxds_gmux_get_client_id( > + struct pci_dev *dev) One line, please. ... > +static acpi_status find_acpi_methods( > + acpi_handle object, u32 nesting_level, void *context, > + void **return_value) Fix indentation here as well. ... > +static int __init mxds_gmux_init(void) > +{ > + int ret = 0; > + struct pci_dev *dev = NULL; Redundant assignment. And keep it in reversed xmas tree order. > + /* Currently only supports Intel integrated and NVIDIA discrete GPUs */ > + while ((dev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) { (Mostly as TODO for somebody else) arch/alpha/kernel/console.c-47- arch/x86/kernel/sysfb_efi.c-117- drivers/acpi/acpi_video.c-2139- drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c:616 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c:288 drivers/gpu/drm/i915/display/intel_acpi.c:136 drivers/gpu/drm/nouveau/nouveau_acpi.c:284 drivers/gpu/drm/radeon/radeon_bios.c:202 drivers/vfio/pci/vfio_pci.c:136 sound/pci/hda/hda_intel.c:1434 (slightly different story) drivers/gpu/drm/qxl/qxl_drv.c-67-static drivers/vfio/pci/vfio_pci.c-153-static So, what I think is better to do in this case is #define for_each_pci_vga(dev) ... in pci.h (at least here for the future use) > + /* Require both integrated and discrete GPUs */ > + if (!ig_dev || !dg_dev) { > + ret = -ENODEV; > + goto done; > + } > + > + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX, > + find_acpi_methods, NULL, NULL, NULL); It's a bit too much. Can we reduce scope somehow? > + /* Require at least one mux */ > + if (!internal_mux_handle && !external_mux_handle) { > + ret = -ENODEV; > + goto done; > + } > + > + ret = vga_switcheroo_register_handler(&handler, 0); > + > +done: > + > + if (ret) { > + pci_dev_put(ig_dev); > + pci_dev_put(dg_dev); > + } > + > + return ret; Can we use usual pattern: ret = ... if (ret) goto error_put_devices; return 0; error_put_devices: pci_dev_put(ig_dev); pci_dev_put(dg_dev); return ret; ? > +} > +module_init(mxds_gmux_init); > + > +static void __exit mxds_gmux_exit(void) > +{ > + vga_switcheroo_unregister_handler(); > + pci_dev_put(ig_dev); > + pci_dev_put(dg_dev); > +} > +module_exit(mxds_gmux_exit); -- With Best Regards, Andy Shevchenko
