Messages in this thread | | | From | Guenter Roeck <> | Subject | Re: [PATCH v3 2/2] hwmon: (asus_wmi_sensors) Support X370 Asus WMI. | Date | Sun, 10 Oct 2021 09:21:03 -0700 |
| |
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@vger.kernel.org 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 >
| |