| Message ID | 20241022091319.82503-1-aichao@kylinos.cn (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] hwmon: (it87) Add support for IT8625E | expand |
On Tue, 2024-10-22 at 17:13 +0800, Ai Chao wrote: > Add support for IT8625E on Centerm P410. > > Signed-off-by: Ai Chao <aichao@kylinos.cn> > --- > change for v2 > - Move IT8625E_DEVID after IT8623E_DEVID > --- > drivers/hwmon/it87.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c > index e233aafa8856..4aeb09f3bfdf 100644 > --- a/drivers/hwmon/it87.c > +++ b/drivers/hwmon/it87.c > @@ -15,6 +15,7 @@ > * IT8620E Super I/O chip w/LPC interface > * IT8622E Super I/O chip w/LPC interface > * IT8623E Super I/O chip w/LPC interface > + * IT8625E Super I/O chip w/LPC interface > * IT8628E Super I/O chip w/LPC interface > * IT8705F Super I/O chip w/LPC interface > * IT8712F Super I/O chip w/LPC interface > @@ -161,6 +162,7 @@ static inline void superio_exit(int ioreg, bool noexit) > #define IT8620E_DEVID 0x8620 > #define IT8622E_DEVID 0x8622 > #define IT8623E_DEVID 0x8623 > +#define IT8625E_DEVID 0x8625 > #define IT8628E_DEVID 0x8628 > #define IT87952E_DEVID 0x8695 > > @@ -2782,6 +2784,7 @@ static int __init it87_find(int sioaddr, unsigned short *address, > case IT8622E_DEVID: > sio_data->type = it8622; > break; > + case IT8625E_DEVID: > case IT8628E_DEVID: > sio_data->type = it8628; > break; 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. Even one of the most basic items is that it will report the wrong chipID in the logs. Regards Frank
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. >> >> Signed-off-by: Ai Chao <aichao@kylinos.cn> >> --- >> change for v2 >> - Move IT8625E_DEVID after IT8623E_DEVID >> --- >> drivers/hwmon/it87.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c >> index e233aafa8856..4aeb09f3bfdf 100644 >> --- a/drivers/hwmon/it87.c >> +++ b/drivers/hwmon/it87.c >> @@ -15,6 +15,7 @@ >> * IT8620E Super I/O chip w/LPC interface >> * IT8622E Super I/O chip w/LPC interface >> * IT8623E Super I/O chip w/LPC interface >> + * IT8625E Super I/O chip w/LPC interface >> * IT8628E Super I/O chip w/LPC interface >> * IT8705F Super I/O chip w/LPC interface >> * IT8712F Super I/O chip w/LPC interface >> @@ -161,6 +162,7 @@ static inline void superio_exit(int ioreg, bool noexit) >> #define IT8620E_DEVID 0x8620 >> #define IT8622E_DEVID 0x8622 >> #define IT8623E_DEVID 0x8623 >> +#define IT8625E_DEVID 0x8625 >> #define IT8628E_DEVID 0x8628 >> #define IT87952E_DEVID 0x8695 >> >> @@ -2782,6 +2784,7 @@ static int __init it87_find(int sioaddr, unsigned short *address, >> case IT8622E_DEVID: >> sio_data->type = it8622; >> break; >> + case IT8625E_DEVID: >> case IT8628E_DEVID: >> sio_data->type = it8628; >> break; > > 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. Guenter
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. 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?
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). > 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. Guenter
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. >> 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 I'll clean up something I used in the past and send an RFC for it.
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
diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c index e233aafa8856..4aeb09f3bfdf 100644 --- a/drivers/hwmon/it87.c +++ b/drivers/hwmon/it87.c @@ -15,6 +15,7 @@ * IT8620E Super I/O chip w/LPC interface * IT8622E Super I/O chip w/LPC interface * IT8623E Super I/O chip w/LPC interface + * IT8625E Super I/O chip w/LPC interface * IT8628E Super I/O chip w/LPC interface * IT8705F Super I/O chip w/LPC interface * IT8712F Super I/O chip w/LPC interface @@ -161,6 +162,7 @@ static inline void superio_exit(int ioreg, bool noexit) #define IT8620E_DEVID 0x8620 #define IT8622E_DEVID 0x8622 #define IT8623E_DEVID 0x8623 +#define IT8625E_DEVID 0x8625 #define IT8628E_DEVID 0x8628 #define IT87952E_DEVID 0x8695 @@ -2782,6 +2784,7 @@ static int __init it87_find(int sioaddr, unsigned short *address, case IT8622E_DEVID: sio_data->type = it8622; break; + case IT8625E_DEVID: case IT8628E_DEVID: sio_data->type = it8628; break;
Add support for IT8625E on Centerm P410. Signed-off-by: Ai Chao <aichao@kylinos.cn> --- change for v2 - Move IT8625E_DEVID after IT8623E_DEVID --- drivers/hwmon/it87.c | 3 +++ 1 file changed, 3 insertions(+)