Re: [PATCH v2] hwmon: (it87) Add support for IT8625E

[Guenter Roeck <linux@roeck-us.net>]

On 10/23/24 15:19, Ahmad Khalifa wrote:
> On 23/10/2024 16:42, Guenter Roeck wrote:
>> On 10/23/24 05:41, Ahmad Khalifa wrote:
>>> On 22/10/2024 14:40, Guenter Roeck wrote:
>>>> On 10/22/24 03:13, Frank Crawford wrote:
>>>>> On Tue, 2024-10-22 at 17:13 +0800, Ai Chao wrote:
>>>>>> Add support for IT8625E on Centerm P410.
>>> ...
>>>>> Can I just add that it isn't a good idea to use the same type for
>>>>> different chips.  There are some specific differences between the
>>>>> chips, which mean that it should have its own entry in
>>>>>
>>>>> static const struct it87_devices it87_devices[]
>>>>>
>>>>> even if currently they are very similar.
>>>>
>>>> According to the information I have, the ADC voltage is different,
>>>> and 8628 supports PECI but 8625 doesn't. Most importantly, 8625
>>>> has multiple register banks. There are also some differences in
>>>> fan control; 8628 can explicitly turn fans off using register bits.
>>>>
>>>> Just mapping the chip to it8628 may be convenient, but it is not
>>>> acceptable.
>>>
>>> Side question here. The standard for an acceptable chip driver is pretty
>>> high (and rightfully so). But a common use case centres around readonly
>>> display of information: temp/fan/in readings. Even just 2-3 readings are
>>> better than nothing.
>>>
>>> Example, I still have to use Frank's out of tree it87 for my IT8688.
>>> It works perfectly fine for me, but still not possible to merge that
>>> device into hwmon's it87.
>>> This IT8625 is another example. The NCT6701D-R will be one more shortly.
>>>
>>
>> For the most part you can use the kernel driver with force_id parameter.
>> That is no different than your suggested patch.
>>
>> Nuvoton is usually very supportive, so I don't see a problem adding
>> support for NCT6701D-R if someone is willing to spend the time to write
>> a driver (or adding support to an existing driver if the chip is similar).
> 
> It's on my radar when they release the mini-ITX X870 board over here.
> Will need to find the datasheet then.
> 

Nuvoton usually makes those available if asked nicely.

>>> A readonly driver that is configurable from userspace would help in that
>>> use case. It can be configured for known devices without datasheets or
>>> for testing new devices. Wouldn't even need to access superio config
>>> space.
>>>
>>> Filesystem has fuse, i2c has i2c-dev, input has evdev, ...
>>> Would something similar be acceptable for hwmon?
>>>
>>
>> I would have to details of a proposal, but It seems unlikely.
>> After all, chip details have to be sufficiently known to provide
>> any driver, even a userspace one. Also, the hwmon ABI (i.s., its set
>> of sysfs entries) isn't that special that it would warrant such a
>> kernel infrastructure. If at all, it might make more sense to add
>> support for this, for example, to libsensors.
> 
> Why not libsensors?
> Access to ACPI and IO ports (without special file capabilities).
> Access to the hwmon sysfs ABI in place of libsensors (as libsensors has
> no movement at all - not the best reason, I know)
> 
> Maybe access to WMI too, but didn't need it in my tests with gigabyte
> WMI, as through ACPI you get fan access in addition to temp
> 

Sorry, you lost me somewhere. Unless I misunderstood your request,
you were asking for a means to support userspace hwmon drivers. Well,
yes, those would somehow need access to the hardware they support.
A userspace hwmon driver for IT8625 will still need to access the hardware.
If those userspace drivers interface with libsensors or with the kernel
does not make a difference. I didn't suggest that this should be implemented
as part of libsensors, but one could define an API which lets userspace
drivers and libsensors communicate with each other.

The lack of a maintainer for libsensors is not an argument to make here.
Anyone can feel free to pick it up.

Guenter