Re: [PATCH v3 2/2] hwmon: (asus_wmi_sensors) Support X370 Asus WMI.
From: Guenter Roeck
Date: Sun Oct 10 2021 - 12:21:15 EST
On 10/10/21 8:28 AM, Thomas Weißschuh wrote:
On 2021-10-10T07:56-0700, Guenter Roeck wrote:
On 10/10/21 7:10 AM, Thomas Weißschuh wrote:
On 2021-10-10T06:38-0700, Guenter Roeck wrote:
On 10/10/21 3:20 AM, Thomas Weißschuh wrote:
Hi,
for WMI drivers the list platform-driver-x86@xxxxxxxxxxxxxxx should probably be
on CC too.
Also all other WMI drivers, even for hwmon stuff are located in
drivers/platform/x86 so it may be better to put it there, too.
Not really. If any of those other drivers are pure hwmon drivers, they
should reside in drivers/hwmon instead. And, yes, that really includes
the gigabyte-wmi driver. We don't have arbitrary drivers in drivers/pci
either just because they are drivers for pci devices.
Fair enough.
I suppose it would be too much churn to move gigabyte-wmi to
hwmon now though, correct?
Is it ? I don't recall the reason why it was added to drivers/platform/x86
in the first place. I see other single-use wmi drivers in that directory
as well (eg xiaomi-wmi, which should be in input). Is there some unwritten
rule stating that all wmi drivers shall reside in drivers/platform/x86,
no matter what subsystem they touch ?
There was no specific reason. I saw all the other WMI drivers in
drivers/platform/x86 and added mine there and sent it to the recipients as
reported by get_maintainer.pl.
Sorry, that is not a valid reason or argument for me.
You mentioned that it could move to hwmon but Hans said there are other
single-use wmi drivers in drivers/platform/x86 so I left it as is.
If you want me to move it, I'd be happy to do so.
At this point this is out of my control.
In any case I think it would make sense to have some sort of written and
well-known policy about this, though.
Absolutely agree. Historically single-use drivers resided in subsystem directories,
and for multi-use drivers it was handled on a case-by-case basis. If that is being
changed (meaning subsystem driver control/maintenance/review is taken away from
subsystem maintainers), it should for sure be documented.
Having the platform-driver-x86 on Cc would still be useful as they can provide
guidance about using the ACPI/WMI/platform APIs.
Sure, but that is unrelated to the driver location, and the opposite argument
can be made as well (that drivers implementing subsystem code should be reviewed
by subsystem maintainers). That is a much stronger argument in my opinion.
Guenter
Absolutely. I wanted to make two different points in my mail:
1) Maybe the driver should be moved into drivers/platform/x86 as the other
(single-use) WMI drivers are living there.
Doing something wrong is neither a reason nor an argument to keep doing it.
I don't know about any rule demanding that but was mentioning this so it stays
consistent.
2) The patch should *also* be reviewed by pdx86 as it is using their
infrastructure.
This was not meant to replace any of the hwmon involvement.
For example when I submitted gigabyte-wmi to pdx86 the maintainers they told
me to also solicit feedback from you as the hwmon maintainer.
And in the end both the hwmon parts (thank you!) and the wmi parts
(platform-device vs WMI bus, same as here) were much better than in the
first version.
Sure. Unfortunately that is not always the case. Many drivers registering
with hwmon were never reviewed by a hwmon maintainer, and some of those make
me shiver when I look at them. Since v5.12, get_maintainer.pl lists hwmon
maintainers as reviewers if a driver makes a hwmon API call, so hopefully
that will help a bit.
Guenter
For example by using the WMI bus as mentioned in my other mail would allow
to completely remove the manually maintained DMI list and instead directly bind
to the WMI GUID for any device that supports this GUID.
(This is possible as this WMI API seems to be self-describing, so all
specific parameters can be discovered by the driver)
Thomas