Re: [LEDE-DEV] Problem with commit "kernel: add hwmon for W83627EHF and family"

2017-05-21 Thread Philip Prindeville 

> On May 21, 2017, at 10:03 AM, Daniel Golle  wrote:
> 
> Hi Rafal,
> 
> On Sun, May 21, 2017 at 05:02:44PM +0200, Rafał Miłecki wrote:
>> Hi,
>> 
>> I noticed commit 0dcc36fc7ddec ("kernel: add hwmon for W83627EHF and
>> family") in the LEDE tree that doesn't look OK to me.
>> 
>> 1) Package for hwmon-w83627ehf
>> Do we need it to be a package? Or could it be built-in into the kernel?
>> Do we need it to be a global package? Usually hwmon drivers are
>> designed for a specific device and it should be enough to have it in
>> target/linux/*/modules.mk
> 
> True, but all other x86-specific hwmon modules are in
> package/kernel/linux/modules as well and I'd rather wanted it to be
> consistent.
> 
>> 
>> 2) 800-hwmon-w83627ehf-dont-claim-nct677x.patch
>> I really don't like this one as it's totally unclear why we need this
>> change. What's wrong with NCT6775 and the w83627ehf driver? It should
>> be well described in the patch.
> 
> Sorry for the lack of a more detailed description.
> The problem here is that the W83627EHF driver also claims the NCT677x
> hardware but doesn't support all features (according to
> Philip Prindeville). I reckon he can provide more information regarding
> which features which are available when using the nct6776 driver are
> missing in the w83627ehf driver on NCT677x hardware.
> 
> Cheers
> 
> Daniel

To follow up on what Daniel said, yes, the older w83627 has a subset of 
functionality for the nct6775 chips, but the native nct6775 driver does a 
better job.

Quoting Documentation/hwmon/nct6775:

Note


This driver supersedes the NCT6775F and NCT6776F support in the W83627EHF
driver.

and drivers/hwmon/Kconfig:

config SENSORS_NCT6775
tristate "Nuvoton NCT6775F and compatibles"
depends on !PPC
select HWMON_VID
help
  If you say yes here you get support for the hardware monitoring
  functionality of the Nuvoton NCT6106D, NCT6775F, NCT6776F, NCT6779D,
  NCT6791D, NCT6792D, NCT6793D, and compatible Super-I/O chips. This
  driver replaces the w83627ehf driver for NCT6775F and NCT6776F.

  This driver can also be built as a module.  If so, the module
  will be called nct6775.

...
config SENSORS_W83627EHF
tristate "Winbond W83627EHF/EHG/DHG/UHG, W83667HG, NCT6775F, NCT6776F"
depends on !PPC
select HWMON_VID
help
  If you say yes here you get support for the hardware
  monitoring functionality of the Winbond W83627EHF Super-I/O chip.

  This driver also supports the W83627EHG, which is the lead-free
  version of the W83627EHF, and the W83627DHG, which is a similar
  chip suited for specific Intel processors that use PECI such as
  the Core 2 Duo. And also the W83627UHG, which is a stripped down
  version of the W83627DHG (as far as hardware monitoring goes.)

  This driver also supports Nuvoton W83667HG, W83667HG-B, NCT6775F
  (also known as W83667HG-I), and NCT6776F.

  This driver can also be built as a module.  If so, the module
  will be called w83627ehf.



If you definitely have NCT6775F hardware, they you don’t want to build both 
drivers into your kernel.

If you want to build a kernel that supports both the NCT6775 and W83627HF 
drivers, then you need this patch so that ONLY the NCT6775 will detect the 
chipset.

It’s not clear why a few of the patches to add partial NCT6775 support to the 
W83627HF driver weren’t backed out after the NCT6775 driver was merged…  
probably an oversight.

-Philip


___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] Problem with commit "kernel: add hwmon for W83627EHF and family"

2017-05-21 Thread Daniel Golle 
Hi Rafal,

On Sun, May 21, 2017 at 05:02:44PM +0200, Rafał Miłecki wrote:
> Hi,
> 
> I noticed commit 0dcc36fc7ddec ("kernel: add hwmon for W83627EHF and
> family") in the LEDE tree that doesn't look OK to me.
> 
> 1) Package for hwmon-w83627ehf
> Do we need it to be a package? Or could it be built-in into the kernel?
> Do we need it to be a global package? Usually hwmon drivers are
> designed for a specific device and it should be enough to have it in
> target/linux/*/modules.mk

True, but all other x86-specific hwmon modules are in
package/kernel/linux/modules as well and I'd rather wanted it to be
consistent.

> 
> 2) 800-hwmon-w83627ehf-dont-claim-nct677x.patch
> I really don't like this one as it's totally unclear why we need this
> change. What's wrong with NCT6775 and the w83627ehf driver? It should
> be well described in the patch.

Sorry for the lack of a more detailed description.
The problem here is that the W83627EHF driver also claims the NCT677x
hardware but doesn't support all features (according to
Philip Prindeville). I reckon he can provide more information regarding
which features which are available when using the nct6776 driver are
missing in the w83627ehf driver on NCT677x hardware.


Cheers


Daniel

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev