i2c: modify lm87 to use auto fan_div

A common problem for end users is setting the fan clock divider for correct operation. This patch removes fan_div sysfs write accessor and implements an automatic fan clock divider selection strategy.

Two states are possible: if the user specifies a valid minimum low speed alarm limit that will set an appropriate fan clock divider, otherwise the driver will seek to a fan clock divider that gives best resolution for current fan speed.

The auto fan clock divider algorithm received extensive testing in the adm9240 driver port, this patch is compile tested but needs hardware test.

Testing: something like this script can be used to verify operation:

#!/bin/bash
#
minpath="/sys/bus/i2c/devices/0-002d/fan1_min"
fanpath="/sys/bus/i2c/devices/0-002d/fan1_input"
read fan < $fanpath
lower00
upper500000
fan_min3
step0
#
function test_cycle ()

{
	fan_min=$lower
	while test $fan_min -lt $upper; do
		echo "$fan_min" > "$minpath"
		sleep 2
		read fan < $fanpath
		(( fan_min=($fan_min * $step + 50) / 100 ))
		(( fan_min=(($fan_min + 50) / 100) * 100 ))
	done
	fan_min=$upper
	while test $fan_min -gt $lower; do
		echo "$fan_min" > "$minpath"
		sleep 2
		read fan < $fanpath
		(( fan_min=($fan_min * 100 + $step / 2) / $step ))
		(( fan_min=(($fan_min + 50) / 100) * 100 ))
	done
}

test_cycle

---

 lm87.c |  107 ++++++++++++++++++++++++++++++++++++-----------------------------
 1 files changed, 60 insertions(+), 47 deletions(-)

@@ -405,63 +405,62 @@ static DEVICE_ATTR(fan##offset##_input, 
 show_fan(1);
 show_fan(2);
 
-static void set_fan_min(struct device *dev, const char *buf, int nr)
+/* write new fan_div: callers must hold data->update_lock */
+static void lm87_write_fan_div(struct i2c_client *client, int nr, u8 fan_div)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct lm87_data *data = i2c_get_clientdata(client);
-	long val = simple_strtol(buf, NULL, 10);
+	u8 reg, old, shf = (nr + 2) << 1;
 
-	down(&data->update_lock);
-	data->fan_min[nr] = FAN_TO_REG(val,
-			    FAN_DIV_FROM_REG(data->fan_div[nr]));
-	lm87_write_value(client, LM87_REG_FAN_MIN(nr), data->fan_min[nr]);
-	up(&data->update_lock);
+	reg = lm87_read_value(client, LM87_REG_VID_FAN_DIV);
+	old = (reg >> shf) & 3;
+	reg &= ~(3 << shf);
+	reg |= fan_div << shf;
+	lm87_write_value(client, LM87_REG_VID_FAN_DIV, reg);
+	dev_dbg(&client->dev, "autoX fan%u old %u new %u   fan_div changed\n",
+			nr + 1, 1 << old, 1 << fan_div);
 }
 
-/* Note: we save and restore the fan minimum here, because its value is
-   determined in part by the fan clock divider.  This follows the principle
-   of least suprise; the user doesn't expect the fan minimum to change just
-   because the divider changed. */
-static ssize_t set_fan_div(struct device *dev, const char *buf,
-		size_t count, int nr)
+/* automatic fan clock divider selection when user sets fan_min */
+static void set_fan_min(struct device *dev, const char *buf, int nr)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm87_data *data = i2c_get_clientdata(client);
 	long val = simple_strtol(buf, NULL, 10);
-	unsigned long min;
-	u8 reg;
 
 	down(&data->update_lock);
-	min = FAN_FROM_REG(data->fan_min[nr],
-			   FAN_DIV_FROM_REG(data->fan_div[nr]));
 
-	switch (val) {
-	case 1: data->fan_div[nr] = 0; break;
-	case 2: data->fan_div[nr] = 1; break;
-	case 4: data->fan_div[nr] = 2; break;
-	case 8: data->fan_div[nr] = 3; break;
-	default:
-		up(&data->update_lock);
-		return -EINVAL;
-	}
+	dev_dbg(&client->dev, "auto? fan%u div %u min %3u val %5ld spd %u\n",
+			nr + 1, 1 << data->fan_div[nr],
+			data->fan_min[nr],val,data->fan[nr]);
+
+	if (val <= 1350000 / (8 * 254)) {
+		data->fan_min[nr] = 255; /* too low, disable alarm */
+		dev_dbg(&client->dev, "auto! fan%u div %u min %3u too low\n",
+				nr + 1, 1 << data->fan_div[nr],
+				data->fan_min[nr]);
+	} else {
+		/* calculate optimum fan clock divider to suit fan_min */
+		unsigned int new_min = 1350000U / val;
+		u8 new_div = 0;
+
+		while (new_min > 192 && new_div < 3) {
+			new_div++;
+			new_min++;
+			new_min >>= 1;
+		}
+		dev_dbg(&client->dev, "auto- fan%u div %u min %3u\n",
+				nr + 1, 1 << new_div, new_min);
 
-	reg = lm87_read_value(client, LM87_REG_VID_FAN_DIV);
-	switch (nr) {
-	case 0:
-	    reg = (reg & 0xCF) | (data->fan_div[0] << 4);
-	    break;
-	case 1:
-	    reg = (reg & 0x3F) | (data->fan_div[1] << 6);
-	    break;
+		data->fan_min[nr] = new_min;
+
+		if (new_div != data->fan_div[nr]) {
+			data->fan_div[nr] = new_div;
+			lm87_write_fan_div(client, nr, new_div);
+		}
 	}
-	lm87_write_value(client, LM87_REG_VID_FAN_DIV, reg);
 
-	data->fan_min[nr] = FAN_TO_REG(min, val);
 	lm87_write_value(client, LM87_REG_FAN_MIN(nr),
 			 data->fan_min[nr]);
 	up(&data->update_lock);
-
-	return count;
 }
 
 #define set_fan(offset) \

@@ -471,15 +470,10 @@ static ssize_t set_fan##offset##_min(str
 	set_fan_min(dev, buf, offset-1); \
 	return count; \
 } \
-static ssize_t set_fan##offset##_div(struct device *dev, const char *buf, \
-		size_t count) \
-{ \
-	return set_fan_div(dev, buf, count, offset-1); \
-} \
 static DEVICE_ATTR(fan##offset##_min, S_IRUGO | S_IWUSR, \
 		show_fan##offset##_min, set_fan##offset##_min); \
-static DEVICE_ATTR(fan##offset##_div, S_IRUGO | S_IWUSR, \
-		show_fan##offset##_div, set_fan##offset##_div);
+static DEVICE_ATTR(fan##offset##_div, S_IRUGO, \
+		show_fan##offset##_div, NULL);
 set_fan(1);
 set_fan(2);
 

@@ -767,6 +761,25 @@ static struct lm87_data *lm87_update_dev
 			} else {
 				data->fan[i] = lm87_read_value(client,
 					       LM87_REG_FAN(i));
+
+				/* if fan_min off: auto fan clock divider */
+				if (data->fan_min[i] = 255) {
+					int x = 0;
+
+					if (data->fan[i] > 192 &&
+						data->fan_div[i] < 3) {
+						x++;
+					}
+					if (data->fan[i] < 96 &&
+						data->fan_div[i] > 0) {
+						x--;
+					}
+					if (x != 0) {
+						data->fan_div[i] += x;
+						lm87_write_fan_div(client, i,
+							data->fan_div[i]);
+					}
+				}
 				data->fan_min[i] = lm87_read_value(client,
 						   LM87_REG_FAN_MIN(i));
 			}

Hi Khali,
On Sat, 16 Apr 2005 11:26:16 +0200, Jean Delvare <[email protected]> wrote:

Second look as I modify the driver to suit your comments.

I tried with and without 'p' diff option, would changing the number of context lines stop diff from 'merging' a replacement function? No, it didn't -- would appreciate a cluebat on getting a better diff?

I like the idea of saying we changed from 'old' to 'new'

Done!

It really helps during testing with the ramping up and down the fan limit setting, to watch the register values being adjusted, removed.

Okay

Okay

Done!

And the other side of the same decision point: 255?

I prefer correctness in measurement systems, I think I'm correct here.

You're right, I now detect change from 'disabled' mode and report "fanX low speed limit enabled", complementary to the above 'disabled'.

Alright, deleted it altogether, sidestep the issue :)

Disagree: we already selected best fan clock divider when setting the low speed limit, cannot change it after that because we lose the alarm point resolution. Adjusting operating point while a limit is set introduced problems in my testing, so I make a simple rule to define how the algorithm works.

Disagree: when using a wide speed range fan user may prefer most accurate fan speed measurement reading.

It is the idea that user explicitly selects either fan low speed limit mode, or wants to monitor fan speed.

New patch as soon as compile check. Still can't get a better looking diff, rearranging the code for a clean diff would be worse, no? :)

Cheers,
Grant.

Hi Grant,

While I like the idea of an automatic selection of the fan clock divider by the driver, I don't think that your implementation is correct.

If there is a low limit set, you do not take the actual speed measurement into account for divider selection. What if the limit has been set significantly higher than the actual fan speed? This speed cannot be measured, and the driver won't adapt the divider. Since the BIOS typically sets an arbitrary low speed limit (or the chip itself has a power-up default), you have the very same problem as with the original model where the user needs to change the divider manually before he/she can get a reading. In other words, your driver modification fails to address the problem.

The fan clock divider increase upon unreadable fan speed (lower than the current divider allows) must happen regardless of the low limit being set or not. If not, then the driver modification isn't worth the effort.

I also believe that the measured fan speed should not possibly lead to a divider decrease. Decreasing the divider will require a min limit register change in the general case, which will result in rounding at best, plain loss of limit at worst.

The model I am using for the upcoming w83627ehf driver is as follow:

1* When the fan speed register reads 255 (overflow), try increasing the divider if possible.

2* When the user sets a low limit, pick the best divider for this speed, where best is defined by "register value between 96 and 191". This ensures that all speeds over the limit can be read with a reasonable accuracy, and speeds down to 75% of the low limit as well.

The advantages are:

1* If the user doesn't configure the chip, he/she will still get valid readings (providing that there is a fan connected, of course) after a limited number of tries.

2* If the user does configure the chip, he/she will set the low limit, and get the best divider (providing that he/she picks a reasonable limit, of course).

3* It is guaranteed by design that the fan clock divider can only change a limited (and low) number of times after it has been loaded or the fan low limit has been changed.

Your implementation fails to provide points 1 and 3.

Some comments on your coding style now, although the patch is quite hard to read (not sure there's anything you can do about this though).

Jean Delvare

Hi Grant,

No idea, unfortunately. I've dreamt of a "--do-not-try-to-be-clever" option to GNU diff, but there isn't. Nevermind, in this case you can look at the new code directly rather than the diff.

What do you mean exactly?

If that was the sense of your question, I agree that in some cases your rounding will happen to do the right thing (e.g. for 2640, your method leads to 2636, mine leads to 2657). My point was that your method would introduce cumulated rounding errors in some cases. I don't think you can argue with that. This is a mathematical fact.

So, either do proper rounding, or don't do any rounding at all. Rounding that will work properly for half of the cases and make things worse for the other half isn't worth the additional effort. As the proper rouding looked significantly more expensive, I opted for no rounding at all. If you come with an elegant, proper rounding solution, I'll consider it.

Also beware that whatever rounding you do should be taken into consideration in the earlier test which discards the corner cases. Failing to do so might result in overflows.

And this is a bad idea. Asking the user to choose between having a low speed alarm and "better" fan speed measurements is stupid. The whole point of hardware monitoring is to detect hardware problems, and you are almost inviting the user to disable the alarms in exchange of better measurements!

Please remember that we are *not* introducing automatic fan clock divider selection to provide stlightly more accurate fan speed readings. We are doing it so that people stop wondering and complaining that their fan speed reads 0 while the fan is spinning. My implementation does just this, while yours fails to address the problem. This is the reason why I can't accept it.

Thanks,

Jean Delvare

Hi Grant,

Agreed. We want to handle all cases but with varying level of effort.

If the user doesn't setup anything, we at least want the fan speed reading to be correct. This is why we have the clock divider increment on 255 fan reading.

If the user setups things in a correct way (relevant low limit), then we want to pick the best divider automatically. This is why we have the divider adjustment in set_fan_min().

If the user setups things in a weird way (ridiculous limit), we don't care. Of course we don't want this to break, but we don't attempt to optimize that case in any way.

In particular, we do not want to do anything special when the low limit is disabled. There are only two valid reasons to disable the limit:
1* No fan is connected. No need to waste our energy on this case, obviously.
2* The fan is actually running slower that the chip supports. The current code will switch to the highest divider, which is the correct thing to do. No additional code is needed.

Disabling the low limit when you have a fan running at a standard speed is a bad idea, which is why I ask you not to add code to support that case.

Thanks,

Jean Delvare

Hi Grant,

That's exactly how things are supposed to work. It improved my understanding as well. My w83627ehf implementation was broken, now it's better.

If you browse the mailing-list archive, you'll find that I started working on automatic clock divider long ago. My first implementation was bad. It improved over time, thanks to the feedback of various people, including you, to become what the w83627ehf has.

To say the truth, well yes, a little bit at times. This happens, no need to worry. You have the energy of the newcomer. I have the experience of the user support. No surprise we don't see everything the same way.

What exactly are you trying to achieve? 192 seemed to be a good equilibrium between range margin and resolution.

Absolutely. Note that this guarantees a "stable" divider, while bidirectional could result in endless divider change, which I would like to avoid, because divider change is somewhat expensive.

Thanks,

Jean Delvare

Hi Grant,

Yes, looks like the best thing to do.

Interesting. I was doing almost that in w83627ehf before (setting fan_min to 255 instead of 254, but still setting div to the max value). What you propose here indeed makes sense. I would then suggest that we do add a warning message as you proposed earlier (fan%u low limit %u below minimum, setting to %u instead).

Yes, that's what I wanted. "fan_min set too high" isn't a special case IMHO. It is handled just fine by the regular code. As said earlier, if the user asks for something stupid, he/she gets what he/she deserves. We can't help.

True, except that these are really two separate rules. "fan_min > 192" is the rule when setting fan low limit, "speed = 255 and div < max_div" is the rule when speed reading overflows.

Thanks,

Jean Delvare

Hi Khali,
On Sun, 17 Apr 2005 08:14:21 +1000, Grant Coady <[email protected]> wrote:

I changed the test generator, it spits out values of interest, rather than the boring 20% test I was using. The generator does rounding and produces a much more interesting test set.

The critical test point is the switch from too low a fan speed limit to a valid low fan speed limit, this is happening on the 664 -> 667 rpm (255 -> 254) transition which is correct for max fan_div = 8 on adm9240.

The 'auto fan clock divider stepper' is disabled during this testing to isolate what we're testing.

Thus I propose that some limit be set on this loss of resolution, where? 5 or 6 bits? 4 bits is still about 8% resolution, going less would be a problem. Plus the user fan speed limit is likely to drift as resolution drops.

The limit I chose was zero change on fan low speed limit setting, though I see there may be some room to move, but how much?

Returning to this document after the last -- I'll try out the idea of forcing correct fan speed display at cost of fan limit resolution, if I can avoid auto fan divider _change_, I will, I'm beginning to think it is wrong track.

So I see it more as me putting wrong priorities on things, thus I now accept your argument of valid alarm + fan speed display, the 'sufferer' will be fan speed resolution, but that does not matter for ridiculous settings, which is what we want handle with "Just Works". Agree?

Cheers,
Grant.

Test generator, just in case I goofed.

#!/bin/bash
#

# test auto fan clock divider algorithm
#
# Copyright (C) 2005 Grant Coady <[email protected]>
#
# GPLv2 per linux/COPYING by reference
#
minpath="/sys/bus/i2c/devices/0-002d/fan1_min"
fanpath="/sys/bus/i2c/devices/0-002d/fan1_input"
values=(92 93 94 95 96 97 98 190 191 192 193 194 253 254 255 256 257 258 259)
#
function test_two ()
{
	local y=0

for fan_div in 1 2 4 8; do
while [ $y -lt 19 ]; do
fan_limit=$(( (1350000 + ($fan_div * ${values[${y}]}) / 2)\
/ ($fan_div * ${values[${y}]}) ))
echo -e "$fan_limit" >> "$TMPFILE"
(( y++ ))
done
y=0
done

sort -n < "$TMPFILE" | uniq > "$TMPFILE.sort"

while read fan_limit rest; do
echo $fan_limit > "$minpath"
sleep 1
read fan_rpm < $fanpath
echo "set fan_limit=$fan_limit, read fan_rpm=$fan_rpm" sleep 1
done < "$TMPFILE.sort"
}

TMPFILE=$(mktemp -t xyz.XXXXXX)
echo "Grant's auto fan_div tester..."
test_two
rm -f $TMPFILE
rm -f $TMPFILE.sort
#end

Hi Khali,

..

Reasons don't matter, user says zero, we disable alarm, no touch fan clock easy rule.

OT:
I lost sight of some things, spent hours yesterday proving negative voltage readings, as they are ratiometric to the positive +5V, not a reference -- dunno if 'sensors' can do that -- then only one of three datasheets had a useful transfer calculation. Sorted the switch "what" for limits, just the display of min/max get swapped, the register settings -- though I did that empirically, rather than prove a transfer function.

okay, yes -- but should we not then return error message in this case? User has set a value and apparently nothing happened, (user doesn't check her logs any :) Why bother?

I see your point know I had the alarm state in sight, we want fan speed, even when fan_min register goes to zero we have alarm state, so it is okay. Just want fan speed too, can do that.

Having to explain this to each other improves my understanding. Hope you not too annoyed with me :)

Something has to take a hit, and fan limit resolution is it, I've already dropped the chop point from 192 down to 128, then check if fan > 192 and do one adjustment in the set_fan_min. Rest of adjustment in measurement side, not bidirectional until we see a need? Hitting fan_min resolution early will (I hope) improve the thing. Getting late my time -- more tomorrow.

With measurements, proof.

Cheers,
Grant.

Hi Khali,

Exploring, it did nothing good for us, back up at 192 :)

Okay, what I have so far is fan_min set to zero, disable fan alarm, no touch divider.

fan_min set too low, set to lowest value (div = 8, fan_min = 254), this indicates to the user the lowest limit value for adm9240: 664. Since their fan is (mine was) running alarm not asserted, alarm will be asserted if fan speed goes below min operating point --> correct operation.

fan_min set too high? 50000 -> fan_min, displayed back as something like 42000 and alarm asserted :) Takes a few measurement cycles to recover fan speed display... that's what you wanted?

Not quite right yet...

The rules seem to be don't auto-adjust fan clock divider unless fan_min > 192, speed = 255 and div < max_div...

Then my brain stopped :) And I just shut down the other machines.

Cheers,
Grant.

Oops, sorry, I included an untested function from elsewhere, please apply this fix over the second round patch. I am testing other changes on adm9240 driver.

# lm87.c |    2 +-
# 1 files changed, 1 insertion(+), 1 deletion(-)

@@ -414,7 +414,7 @@ static void lm87_write_fan_div(struct i2
 	reg = lm87_read_value(client, LM87_REG_VID_FAN_DIV);
 	old = (reg >> shift) & 3;
 	reg &= ~(3 << shift);
-	reg |= fan_div << shift;
+	reg |= (fan_div << shift);
 	lm87_write_value(client, LM87_REG_VID_FAN_DIV, reg);
 	dev_dbg(&client->dev, "fan%d clock divider changed from %u to %u\n",
 					nr + 1, 1 << old, 1 << fan_div);

Hi Khali,
On Sat, 16 Apr 2005 03:36:44 +1000, Grant Coady <[email protected]> wrote:

Here is round two, modified for most of your comments.

Re: set_fan_min, it may be all I've done is needlessly shift the decision point, so I changed it to match your algo.

I fully agree on the reporting messages, they were geared to my testing.

Changed the fan measurement logic to remove use of local variable.

I'm less happy about changing the fan clock divider in low speed limit alarm mode, I'll need to try it out on adm9240 driver before changing algorithm elsewhere.

Compile tested.

# lm87.c |  116 +++++++++++++++++++++++++++++++++++++----------------------------
# 1 files changed, 67 insertions(+), 49 deletions(-)

@@ -405,63 +405,68 @@ static DEVICE_ATTR(fan##offset##_input, 
 show_fan(1);
 show_fan(2);
 
-static void set_fan_min(struct device *dev, const char *buf, int nr)
+/* write new fan_div: callers must hold data->update_lock */
+static void lm87_write_fan_div(struct i2c_client *client, int nr, u8 fan_div)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct lm87_data *data = i2c_get_clientdata(client);
-	long val = simple_strtol(buf, NULL, 10);
+	u8 reg, old;
+	u8 shift = (nr + 2) << 1; /* nr 0,1 -> 4,6 bit shift count */
 
-	down(&data->update_lock);
-	data->fan_min[nr] = FAN_TO_REG(val,
-			    FAN_DIV_FROM_REG(data->fan_div[nr]));
-	lm87_write_value(client, LM87_REG_FAN_MIN(nr), data->fan_min[nr]);
-	up(&data->update_lock);
+	reg = lm87_read_value(client, LM87_REG_VID_FAN_DIV);
+	old = (reg >> shift) & 3;
+	reg &= ~(3 << shift);
+	reg |= fan_div << shift;
+	lm87_write_value(client, LM87_REG_VID_FAN_DIV, reg);
+	dev_dbg(&client->dev, "fan%d clock divider changed from %u to %u\n",
+					nr + 1, 1 << old, 1 << fan_div);
 }
 
-/* Note: we save and restore the fan minimum here, because its value is
-   determined in part by the fan clock divider.  This follows the principle
-   of least suprise; the user doesn't expect the fan minimum to change just
-   because the divider changed. */
-static ssize_t set_fan_div(struct device *dev, const char *buf,
-		size_t count, int nr)
+/* set fan low speed limit
+ * 
+ * # if user selects a valid fan low speed limit the driver enables low
+ *   fan speed limit alarm and selects a fan clock divider to suit the 
+ *   low speed limit point
+ *
+ * # if user selects a value too low to measure, disable fan low speed 
+ *   limit alarm mode, this will also enable automatic fan clock divider
+ *   adjustment in the fan speed measurement code */
+static void set_fan_min(struct device *dev, const char *buf, int nr)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm87_data *data = i2c_get_clientdata(client);
 	long val = simple_strtol(buf, NULL, 10);
-	unsigned long min;
-	u8 reg;
 
 	down(&data->update_lock);
-	min = FAN_FROM_REG(data->fan_min[nr],
-			   FAN_DIV_FROM_REG(data->fan_div[nr]));
-
-	switch (val) {
-	case 1: data->fan_div[nr] = 0; break;
-	case 2: data->fan_div[nr] = 1; break;
-	case 4: data->fan_div[nr] = 2; break;
-	case 8: data->fan_div[nr] = 3; break;
-	default:
-		up(&data->update_lock);
-		return -EINVAL;
-	}
 
-	reg = lm87_read_value(client, LM87_REG_VID_FAN_DIV);
-	switch (nr) {
-	case 0:
-	    reg = (reg & 0xCF) | (data->fan_div[0] << 4);
-	    break;
-	case 1:
-	    reg = (reg & 0x3F) | (data->fan_div[1] << 6);
-	    break;
+	if (val < 1350000U / (8 * 254)) {
+		if (data->fan_min[nr] < 255) { /* only report on change */
+			data->fan_min[nr] = 255;
+			dev_dbg(&client->dev, "fan%d low speed limit disabled\n",
+					nr + 1);
+		}
+	} else {
+		/* calculate optimum fan clock divider to suit new fan_min */
+		unsigned int new_min = 1350000U / val;
+		u8 new_div = 0;
+
+		while (new_min > 192 && new_div < 3) {
+			new_div++;
+			new_min >>= 1;
+		}
+		
+		if (data->fan_min[nr] = 255) { /* only report on change */
+			dev_dbg(&client->dev, "fan%d low speed limit enabled\n",
+					 nr + 1);
+		}
+		data->fan_min[nr] = new_min;
+		
+		if (data->fan_div[nr] != new_div) {
+			data->fan_div[nr] = new_div;
+			lm87_write_fan_div(client, nr, new_div);
+		}
 	}
-	lm87_write_value(client, LM87_REG_VID_FAN_DIV, reg);
-
-	data->fan_min[nr] = FAN_TO_REG(min, val);
 	lm87_write_value(client, LM87_REG_FAN_MIN(nr),
 			 data->fan_min[nr]);
 	up(&data->update_lock);
-
-	return count;
 }
 
 #define set_fan(offset) \

@@ -471,15 +476,10 @@ static ssize_t set_fan##offset##_min(str
 	set_fan_min(dev, buf, offset-1); \
 	return count; \
 } \
-static ssize_t set_fan##offset##_div(struct device *dev, const char *buf, \
-		size_t count) \
-{ \
-	return set_fan_div(dev, buf, count, offset-1); \
-} \
 static DEVICE_ATTR(fan##offset##_min, S_IRUGO | S_IWUSR, \
 		show_fan##offset##_min, set_fan##offset##_min); \
-static DEVICE_ATTR(fan##offset##_div, S_IRUGO | S_IWUSR, \
-		show_fan##offset##_div, set_fan##offset##_div);
+static DEVICE_ATTR(fan##offset##_div, S_IRUGO, \
+		show_fan##offset##_div, NULL);
 set_fan(1);
 set_fan(2);
 

@@ -767,6 +767,24 @@ static struct lm87_data *lm87_update_dev
 			} else {
 				data->fan[i] = lm87_read_value(client,
 					       LM87_REG_FAN(i));
+
+				/* adjust fan clock divider for best fan 
+				 * speed measurement resolution while
+				 * low fan speed limit alarm disabled */
+				if (data->fan_min[i] = 255) {
+					if (data->fan[i] > 192 &&
+						data->fan_div[i] < 3) {
+						data->fan_div[i]++;
+						lm87_write_fan_div(client, i,
+							data->fan_div[i]);
+					}
+					if (data->fan[i] < 96 &&
+						data->fan_div[i] > 0) {
+						data->fan_div[i]--;
+						lm87_write_fan_div(client, i,
+							data->fan_div[i]);
+					}
+				}
 				data->fan_min[i] = lm87_read_value(client,
 						   LM87_REG_FAN_MIN(i));
 			}

Hi Khali,

This is the issue I was chasing in data-loss -- we assume user knows what they want and set the divider appropriately, the fan may yet to be installed. IMHO this is correct operation. If the fan stops we report an alarm. Alarm status is always valid in this model.

User either does:

nothing: who cares?

sets a limit: we accept their command, alarm status is correct
even if the driver cannot display fan speed

set 0 limit: User is telling us to just display the fan speed,
thus driver may change divider in either direction, I want this feature as it makes sense for wide operating range thermal fans I use. Because the machine with that fan has no fan speed controller in software.

No, I don't think you see the issue.

No, explained above

Until you go one step too far??

I do that, same numbers

No point, if an alarm limit is specified and the fan runs slow, that is an alarm. FULL STOP -- as long as the alarm flag is correct it does not matter if the fan speed is not displayed. So under these conditions we certainly MUST NOT change user limit point. No point having an alarm?

It does not. You have it backwards, as I did for some time, se below on measurement resolution issue.

I have a bad habit of lining up names by size, so I pick 3 letter names for temps, I'll go to one letter temps? CodingStyle. :)

I don't have a non-debug mode, the user selects that from kernel menuconfig?? I make assumption on that...

Too frigging long on the screen, I use 96 column terminals -- was looking at this display updating once every 2 seconds for days... :)

It tells what the driver sees, can go now if you don't want to check operation of the thing.

I get correct operation on ten times overange, divider goes to zero.

okay

Because I shrank the messages and lined them up for data collection.

It does not accumulate errors. It is the same half bit rounding used elsewhere.

Considering particular case is stupid because of the translation through the chip, rarely does the displayed value match the setpoint, nature of the beast, your setting numbers to 1/3000 resolution through a system with 1/254 resolutiuon.

See above

It is there because it works.

can go, was when I was debugging decision point, which now you cannot see because I removed some of the instrumentation already

the variable is alive for three decisions half a screen of code, gimme a break.

No. Not correct, we been there done that.

If I pull the instrumentation, that means nobody else can verify correct operation. What then?

Cheers,
Grant.

Hi Khali,

Centre the decision point to the middle of the bit, basic measurement.

I can easily counter that:

imagine the value 15 / 8

Your method (x >> 3) results in 1, proper rounding results in 2 :)

I argue that 15/8 is two. You argue 7/8 = 0. o_O

I do proper binary rounding :)

We need a way to resolve this:
if the user set too low value, disable alarms

if the user sets too high value, reduce fan limit resolution to guarantee display of fan speed -- thus if user sets stupid alarm point we only ... need ... to ... assert the alarm, gotcha!

fan speed resolution goes out the window, as long as alarm state stays valid this will work. Okay. "Shift the focus, the vision will become clear". Famous person...

My argument is if user sets a known limit they know what they want.

Okay, I've decided: stupid alarm settings are harmless as long as we correctly show alarm state, we trade low speed resolution for displaying fan speed. Okay. Priority is fan speed display and correct alarm state, and we trade unwanted limit resoultion to get it 'cos stupid values only need one bit to say they're stupid.

Earlier I wrote:
What I'm exploring is intentionally reducing the resolution of the alarm setting to allow a wider speed indication range.

On the right track then.

Cheers,
Grant.