> -----Original Message----- > From: Mika Westerberg [mailto:mika.westerberg@xxxxxxxxxxxxxxx] > Sent: Thursday, September 7, 2017 1:50 AM > To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx> > Cc: dvhart@xxxxxxxxxxxxx; LKML <linux-kernel@xxxxxxxxxxxxxxx>; platform-driver- > x86@xxxxxxxxxxxxxxx; Richard Hughes <hughsient@xxxxxxxxx>; Yehezkel Bernat > <yehezkelshb@xxxxxxxxx> > Subject: Re: [PATCH] Add driver to force WMI Thunderbolt controller power status > > On Wed, Sep 06, 2017 at 12:54:00PM -0500, Mario Limonciello wrote: > > Current implementations of Intel Thunderbolt controllers will go > > into a low power mode when not in use. > > > > Many machines containing these controllers also have a GPIO wired up > > that can force the controller awake. This is offered via a ACPI-WMI > > interface intended to be manipulated by a userspace utility. > > > > This mechanism is provided by Intel to OEMs to include in BIOS. > > It uses an industry wide GUID that is populated in a separate _WDG > > entry with no binary MOF. > > > > This interface allow software such as fwupd to wake up thunderbolt > > controllers to query the firmware version or flash new firmware. > > > > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxxx> > > --- > > MAINTAINERS | 5 ++ > > drivers/platform/x86/Kconfig | 13 ++++ > > drivers/platform/x86/Makefile | 1 + > > drivers/platform/x86/intel-wmi-thunderbolt.c | 97 > ++++++++++++++++++++++++++++ > > 4 files changed, 116 insertions(+) > > create mode 100644 drivers/platform/x86/intel-wmi-thunderbolt.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 1c3feff..375bea9 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -3949,6 +3949,11 @@ M: Pali Rohár <pali.rohar@xxxxxxxxx> > > S: Maintained > > F: drivers/platform/x86/dell-wmi.c > > > > +INTEL WMI THUNDERBOLT DRIVER > > +M: Mario Limonciello <mario.limonciello@xxxxxxxx> > > +S: Maintained > > +F: drivers/platform/x86/intel-wmi-thunderbolt.c > > + > > DELTA ST MEDIA DRIVER > > M: Hugues Fruchet <hugues.fruchet@xxxxxx> > > L: linux-media@xxxxxxxxxxxxxxx > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > > index 80b8795..6670a8d 100644 > > --- a/drivers/platform/x86/Kconfig > > +++ b/drivers/platform/x86/Kconfig > > @@ -658,6 +658,19 @@ config WMI_BMOF > > To compile this driver as a module, choose M here: the module will > > be called wmi-bmof. > > > > +config INTEL_WMI_THUNDERBOLT > > + tristate "Intel WMI thunderbolt driver" > > "Intel WMI Thunderbolt force power driver" Adjusted. > > > + depends on ACPI_WMI > > + default ACPI_WMI > > + ---help--- > > + Say Y here if you want to be able to use the WMI interface on select > > + systems to force the power control of Intel Thunderbolt controllers. > > + This is useful for updating the firmware when devices are not plugged > > + into the controller. > > + > > + To compile this driver as a module, choose M here: the module will > > + be called intel-wmi-thunderbolt. > > + > > config MSI_WMI > > tristate "MSI WMI extras" > > depends on ACPI_WMI > > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > > index 91cec17..2b315d0 100644 > > --- a/drivers/platform/x86/Makefile > > +++ b/drivers/platform/x86/Makefile > > @@ -39,6 +39,7 @@ obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o > > obj-$(CONFIG_SURFACE3_WMI) += surface3-wmi.o > > obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o > > obj-$(CONFIG_WMI_BMOF) += wmi-bmof.o > > +obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o > > > > # toshiba_acpi must link after wmi to ensure that wmi devices are found > > # before toshiba_acpi initializes > > diff --git a/drivers/platform/x86/intel-wmi-thunderbolt.c > b/drivers/platform/x86/intel-wmi-thunderbolt.c > > new file mode 100644 > > index 0000000..98f60f2 > > --- /dev/null > > +++ b/drivers/platform/x86/intel-wmi-thunderbolt.c > > @@ -0,0 +1,97 @@ > > +/* > > + * WMI Thunderbolt driver > > + * > > + * Copyright (C) 2017 Dell Inc. All Rights Reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify it > > + * under the terms 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. > > + */ > > + > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > + > > +#include <linux/acpi.h> > > +#include <linux/device.h> > > +#include <linux/fs.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/string.h> > > +#include <linux/sysfs.h> > > +#include <linux/types.h> > > +#include <linux/wmi.h> > > + > > +#define INTEL_WMI_THUNDERBOLT_GUID "86CCFD48-205E-4A77-9C48- > 2021CBEDE341" > > + > > Remove extra newline. Removed. > > > + > > +static ssize_t force_power_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + struct acpi_buffer input; > > + acpi_status status; > > + u8 mode; > > + > > + input.length = (acpi_size) (sizeof(u8)); > > Is this cast really needed? Nope, adjusted. > > > + input.pointer = &mode; > > + mode = hex_to_bin(buf[0]); > > + if (mode == 0 || mode == 1) { > > + status = wmi_evaluate_method(INTEL_WMI_THUNDERBOLT_GUID, > 0, 1, > > + &input, NULL); > > + if (ACPI_FAILURE(status)) { > > + pr_err("intel-wmi-thunderbolt: failed setting %s\n", > > + buf); > > + return -ENODEV; > > + } > > + } else { > > + pr_err("intel-wmi-thunderbolt: unsupported mode: %d", mode); > > + } > > + return count; > > +} > > + > > +static DEVICE_ATTR_WO(force_power); > > + > > +static struct attribute *tbt_attrs[] = { > > Can this be const? No, "initialization from incompatible pointer type" if set like that. Other drivers in platform/ seem to follow the same approach too. > > > + &dev_attr_force_power.attr, > > + NULL > > +}; > > + > > +static const struct attribute_group tbt_attribute_group = { > > + .attrs = tbt_attrs, > > +}; > > + > > +static int intel_wmi_thunderbolt_probe(struct wmi_device *wdev) > > +{ > > + return sysfs_create_group(&wdev->dev.kobj, &tbt_attribute_group); > > You need to document this in Documentation/ABI. While there, I think it > make sense to update Documentation/admin-guide/thunderbolt.rst > accordingly. > Thanks, will fix for v2. > Also you are adding an attribute to a device that is already announced > to the userspace (I think). So it is possible userspace does not find > this when it deals with the uevent. Not sure if it is really a problem > in this case, though. > I don't think it will matter in this case. It will come down to how fwupd decides to use this (which will be TBD and is under discussion still). > Other than that, looks nice to me :) Thanks for your feedback. I'll send a follow up v2 in a few moments.
