On 4/8/21 9:07 AM, Hans de Goede wrote: > Hi Guenter, > > On 4/8/21 5:08 PM, Guenter Roeck wrote: >> On Mon, Apr 05, 2021 at 10:48:10PM +0200, Thomas Weißschuh wrote: >>> Changes since v1: >>> * Incorporate feedback from Barnabás Pőcze >>> * Use a WMI driver instead of a platform driver >>> * Let the kernel manage the driver lifecycle >>> * Fix errno/ACPI error confusion >>> * Fix resource cleanup >>> * Document reason for integer casting >>> >>> Thank you Barnabás for your review, it is much appreciated. >>> >>> -- >8 -- >>> >>> Tested with a X570 I Aorus Pro Wifi. >>> The mainboard contains an ITE IT8688E chip for management. >>> This chips is also handled by drivers/hwmon/i87.c but as it is also used >>> by the firmware itself it needs an ACPI driver. >>> >>> Unfortunately not all sensor registers are handled by the firmware and even >>> less are exposed via WMI. >>> >>> Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx> >>> --- >>> drivers/platform/x86/Kconfig | 11 +++ >>> drivers/platform/x86/Makefile | 1 + >>> drivers/platform/x86/gigabyte-wmi.c | 138 ++++++++++++++++++++++++++++ >> >> Originally drivers/platform was supposed to be used for platform specific >> code. Not that I have control over it, but I really dislike that more and >> more hwmon drivers end up there. >> >> At least hwmon is in good company - I see drivers for various other subsystems >> there as well. I just wonder if that is such a good idea. That entire directory >> is bypassing subsystem maintainer reviews. > > In case you are not aware I've recent(ish) taken over the drivers/platform/x86 > maintainership from Andy Shevchenko. > > Yes it is a bit of an odd grab-bag it mostly deals with vendor specific > ACPI / WMI interfaces which often more or less require using a single > driver while offering multiple functionalities. These firmware interfaces > do not really lend themselves to demultiplexing through something like > MFD. These are mostly found on laptops where they deal with some or all of: > > - Hotkeys for brightness adjust / wlan-on/off toggle, touchpad on/off toggle, etc. > (input subsystem stuff) > - Mic. / Speaker mute LEDS (and other special LEDs) found on some laptops > (LED subsystem stuff) > - Enabling/disabling radios > (rfkill stuff) > - Controlling the DPTF performance profile > (ACPI stuff) > - Various sensors, some hwmon, some IIO > - Backlight control (drm/kms subsys) > - Enabling/disabling of LCD-builtin privacy filters (requires KMS/DRM subsys integration, pending) > - Fan control (hwmon subsys) > > And often all of this in a single driver. This is all "stuff" for which > there are no standard APIs shared between vendors, so it is a free for > all and often it is all stuffed behind a single WMI or ACPI object, > because that is how the vendor's drivers under Windows work. > > It certainly is not my intention to bypass review by other subsystem > maintainers and when there are significant questions I do try to always > get other subsys maintainers involved. See e.g. this thread, but also the > "[PATCH 1/3] thinkpad_acpi: add support for force_discharge" thread > where I asked for input from sre for the power-supply aspects of that. > > The WMI code was reworked a while back to make WMI be a bus and have > individual WMI objects be devices on that bus. version 2 of this > driver has been reworked to use this. Since this new driver is just a hwmon > driver and as this is for a desktop I expect it will stay that way, > I'm fine with moving this one over to drivers/hwmon if that has your > preference. > I thought about it, but I don't think it makes much sense since all other WMI drivers are in drivers/platform. > As for other cases then this driver, if you want to make sure you are at > least Cc-ed on all hwmon related changes I'm fine with adding you as a > reviewer to the pdx86 MAINTAINERS entry. > I think I have a better idea: I'll add a regex pattern into the MAINTAINERS entry for hardware monitoring drivers. This way we should get copied whenever anyone adds a hardware monitoring driver into the tree. Thanks, Guenter
