svn repo lm-sensors/trunk lacks w83627dhg support

["David Holl" <david@…>]

I'm attaching a patch (svn diff) of the edits to add the W83627DHG to libsensors, sensors, and sensord. I basically modeled the new snippets after the W83627EHF but removed any reference to voltage sensor in9, because the DHG has 1 less voltage sensor. If you'd prefer, I could try to maximize the code overlap between the EHF & DHG if yall think that'll make the code easier to maintain in the long run. But for now, I just made the DHG functions & constants completely separate from (but near identical to) their EHF equivalents.

[khali]

This is quite a big patch considering that both chips (W83627EHF and W83627DHG) have almost the same feature set. I'd prefer a minimal change not adding a new structure but reusing the one we have for the w83627ehf. We do that for many other chips already.

As a side note: no C++-style comments please.

[ticket]

Replying to khali

From my understanding, the DHG doesn't have voltage monitor in9 which is declared in the EHF's feature list, w83627ehf_features. In the libsensors API, if someone calls sensors_get_all_features(chip_name, ..., ...), is there a general tactic to hide the in9* declarations? Otherwise, 3rd party apps using libsensors would get the false idea that the DHG has in9.

[ticket]

I just attached a very minimalistic patch in that I tried to maximally reuse the existing #define's & structures from the EHF. I'll warn that I think it has a bug where any 3rd party calling sensors_get_all_features may get the wrong impression the DHG has an in9 voltage input. I'm not sure of a clean way to fix that without reeking.

One idea is to move the four (in9,in9_alarm,in9_min,in9_max) declarations in lib/chips.c w83627ehf_features to the front of the list, and then using w83627ehf_features+4 for the DHG's entry in sensors_chip_features_list -->

sensors_chip_features sensors_chip_features_list[] =
{
  /* ... */
  { SENSORS_W83627EHF_PREFIX, w83627ehf_features },
  { SENSORS_W83627DHG_PREFIX, w83627ehf_features+4 }, /* +4 skips the four in9* declarations which are not used on the DHG */
  /* ... */
  { 0 }
};

What do you think?

[khali]

Replying to ticket:

Otherwise, 3rd party apps using libsensors would get the false idea that the DHG has in9.

Your analysis is correct, but there are many other chip declarations with the same problem already. This is essentially a design flow in libsensors, that it requires specific declarations for every new chip we want to support. The issue is even broader, as sometimes a given chip may or may not have a given feature depending on how it was wired and configured on a specific board. We can't possibly add a new structure for every combination of options.

But this is being worked on, we should soon have generic code that can discover the features from each device's exposed sysfs interface, so static declarations are no longer necessary. In the meantime, we have to live with what we have. At the moment, 3rd party applications can't really use generic code to display the chip features anyway, in practice they need dedicated code, so the improperly declared w83627dhg's in9 is no big deal.

[khali]

Replying to ticket:

I just attached a very minimalistic patch in that I tried to maximally reuse the existing #define's & structures from the EHF. I'll warn that I think it has a bug where any 3rd party calling sensors_get_all_features may get the wrong impression the DHG has an in9 voltage input. I'm not sure of a clean way to fix that without reeking.

I've applied your patch to our source repository, thanks for your contribution. Don't worry about in9, it's really not a big problem in practice.

[ticket]

Eww! :) Damn that makes it hard to encourage people to use libsensors instead of /sys for the automagic device naming/scaling provided with sensors.conf.

Hmm... a simpler fix (for now) -- should we place create a w83627dhg chip section in sensor.conf.eq with "ignore in9"?

- David