lkml.org 
[lkml]   [2021]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v3 2/2] hwmon: (asus_wmi_sensors) Support X370 Asus WMI.
Date
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
>

\
 
 \ /
  Last update: 2021-10-10 18:22    [W:0.187 / U:0.280 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site