On Wed, Dec 15, 2010 at 07:29:15AM -0500, R, Durgadoss wrote: > Hi Guenter, > > > > ------------------------------------------------------------------ > > > From: Durgadoss R <durgadoss.r@xxxxxxxxx> > > > > > > Date: Tue, 14 Dec 2010 05:10:27 +0530 > > > Subject: [PATCH] Adding_threshold_support_to_coretemp > > > > > > This patch adds the core thermal threshold support to coretemp.c. > > > These thresholds can be read/written using the sysfs interface > > > temp1_core_thresh[0/1]. These can be used to generate interrupts, > > > to do dynamic power management. > > > > > > Signed-off-by: Durgadoss R <durgadoss.r@xxxxxxxxx> > > > > > I am not inclined to look further into this patch until the ABI questions are > > resolved. > > As written, the patch gets my NACK simply because it introduces random driver > > specific > > new ABI attributes. > > > > Since the Hardware supports the programming of thresholds, I added this support > in the driver. I had to add new attributes, since there are no existing attributes, > that support these thresholds. > Could you please suggest any better way(s) of enabling this support ? > I can add documentation for these attributes in Doc/hwmon/sysfs-interface. > At least two. 1) Use existing ABI attributes. The coretemp driver doesn't use all available attributes for low/high temperatures, so you could pick unused ones (and if necessary redefine the ones already used). We have lcrit min max crit emergency All those attributes are ultimately thresholds; no reason not to use them. Furthermore, the two thresholds are really upper/lower temperature targets which result in requesting a specific action. As such, it is really just one (upper) threshold with an associated hysteresis value to undo the action caused by the upper threshold. So you could use tempX_max tempX_max_hyst or tempX_crit tempX_crit_hyst or tempX_emergency tempX_emergency_hyst instead to reflect this meaning. I don't know how the "THRESHOLD" and the "TARGET" temperature relate to each other. Maybe you could simply use tempX_max as upper threshold (where it exists), with MSR_IA32_TEMPERATURE_TARGET as initial value, and define and use tempX_max_hyst to calculate the lower threshold. 2) Define new attribute names which can be used as generic ABI, such as tempX_threshold tempX_threshold_hyst Right now I don't really see the need for additional attributes, since there are spare ones available, so 2) is less likely to be accepted. You would have to make a really good case for it, and not just to me but convince Jean as well. Thanks, Guenter _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
