Hi Jean, thanks for the feedback.Regarding the SENSORS_LIMIT(reqval, 0, 0xffff) in store_in8() which you have tagged as spurious, I believe it is necessary to prevent an overflow situation if an input value were provided such that reqval * 0xc0 > LONG_MAX. I should probably have added a comment to this effect. The second SENSORS_LIMIT() then limits the result of the scaling calculation to the register range 0 - 0xff. To get away with a single SENSORS_LIMIT(), it would be necessary guard to against the full-scale value mapping to 256, which won't fit in the register. This could be accomplished like this:
SENSORS_LIMIT(reqval, 0, ((asc7621_in_scaling[nr] * 0x100) / 0xc0) - 1);- but that just looks far too obscure to me, and not worth the bother. Please let me know if you think otherwise.
In general I've been very cautious in preparing the patch to only change lines of code that were demonstrably broken, rather than fixing up stylistic issues. For example, the temperature scaling calculation is done in what is IMHO a rather bizarre fashion, but it doesn't actually get the wrong answers so I have left well alone. The same goes for use of signed and unsigned types: personally, I'd strongly prefer to see long, simple_strtol() and sprintf("%ld") used throughout the driver for the scaled values, as I think consistency makes for more maintainable code. But I'm happy to go with your suggested changes all the same.
Lastly, regarding the PWM control settings, I will take a look at what can be done to make these comply better with the sysfs-interface. IMHO there is no completely clean solution here; several of the sysfs-interface settings map onto the same bits of the same registers in the device, so they cannot be completely orthogonalised.
On a note of more general discussion: I don't want to sound negative, but maybe the PWM control interface has been a step too far for the hwmon drivers. For a device like the asc7621, the programmer has to bend over backwards to make it fit the published interface, simply because the settings must be poked into the device one at a time as they are changed. For this kind of device, it would be better to build up a complete register profile and then stuff the whole thing into the device in one go. This could be better handled by a userland library than a kernel driver... Opinionated rant over. ;-)
Best wishes, -Ken. On 03/05/10 11:58, Jean Delvare wrote:
Hi Ken, On Sat, 01 May 2010 14:08:20 +0100, Ken Milmore wrote:Find patch attached; this should apply against Linus' 2.6 tree. The issues fixed here have all been discussed before on the lm_sensors list (see 07/2008 and 04/2010), so hopefully they are uncontroversial.Thanks for the patch. Review below.Having taken a brief look at the PWM control inteface in the driver, I can see there are a few rough edges there but it is probably too late to address them now. In particular the pwm[1-3]_enable and pwm[1-3]_auto_channels settings tend to interpret "Fan control disabled" as "Fan off", which doesn't accord with the principle of least surprise, or with the sysfs-interface documentation. IMHO "Fan full on" would have been a much safer default for these settings...Indeed, and this is serious enough that I would take a patch fixing that right now. Especially pwm[1-3]_enable == 0 MUST switch fans to full speed, pwmconfig and fancontrol rely on this.From 5a9cc351ca11375a81082a06212ac45ac900b213 Mon Sep 17 00:00:00 2001 From: Ken Milmore<ken.milmore@xxxxxxxxxxxxxx> Date: Sat, 1 May 2010 13:26:41 +0100 Subject: [PATCH] hwmon: (asc7621) Bug fixes * Allow fan minimum RPM to be set to zero without triggering alarms. * Fix voltage scaling arithmetic and correct scale factors. * Correct fan1-fan4 alarm bit shifts. * Correct register address for temp3_smoothing_enable. * Read the alarm registers with high priority. Signed-off-by: Ken Milmore<ken.milmore@xxxxxxxxxxxxxx> --- drivers/hwmon/asc7621.c | 63 +++++++++++++++++++++++------------------------ 1 files changed, 31 insertions(+), 32 deletions(-) diff --git a/drivers/hwmon/asc7621.c b/drivers/hwmon/asc7621.c index 7f94810..0f388ad 100644 --- a/drivers/hwmon/asc7621.c +++ b/drivers/hwmon/asc7621.c @@ -268,8 +268,11 @@ static ssize_t store_fan16(struct device *dev, if (strict_strtol(buf, 10,&reqval))It would make sense to use strtoul here instead. Negative fan speeds make no sense.return -EINVAL; + /* If a minimum RPM of zero is requested, then we set the register to + 0xffff. This value allows the fan to be stopped completely without + generating an alarm. */ reqval = - (SENSORS_LIMIT((reqval)<= 0 ? 0 : 5400000 / (reqval), 0, 65534)); + (reqval<= 0 ? 0xffff : SENSORS_LIMIT(5400000 / reqval, 0, 0xfffe));And then this becomes == 0.mutex_lock(&data->update_lock); data->reg[param->msb[0]] = (reqval>> 8)& 0xff; @@ -285,8 +288,9 @@ static ssize_t store_fan16(struct device *dev, * Voltages are scaled in the device so that the nominal voltage * is 3/4ths of the 0-255 range (i.e. 192). * If all voltages are 'normal' then all voltage registers will - * read 0xC0. This doesn't help us if we don't have a point of refernce. - * The data sheet however provides us with the full scale value for each + * read 0xC0. + * + * The data sheet provides us with the 3/4 scale value for each voltage * which is stored in in_scaling. The sda->index parameter value provides * the index into in_scaling. * @@ -295,7 +299,7 @@ static ssize_t store_fan16(struct device *dev, */ static int asc7621_in_scaling[] = { - 3320, 3000, 4380, 6640, 16000 + 2500, 2250, 3300, 5000, 12000 }; static ssize_t show_in10(struct device *dev, struct device_attribute *attr, @@ -306,19 +310,12 @@ static ssize_t show_in10(struct device *dev, struct device_attribute *attr, u8 nr = sda->index; mutex_lock(&data->update_lock); - regval = (data->reg[param->msb[0]] * asc7621_in_scaling[nr]) / 256; - - /* The LSB value is a 2-bit scaling of the MSB's LSbit value. - * I.E. If the maximim voltage for this input is 6640 millivolts then - * a MSB register value of 0 = 0mv and 255 = 6640mv. - * A 1 step change therefore represents 25.9mv (6640 / 256). - * The extra 2-bits therefore represent increments of 6.48mv. - */ - regval += ((asc7621_in_scaling[nr] / 256) / 4) * - (data->reg[param->lsb[0]]>> 6); - + regval = (data->reg[param->msb[0]]<< 8) | (data->reg[param->lsb[0]]); mutex_unlock(&data->update_lock); + /* The LSB value is a 2-bit scaling of the MSB's LSbit value. */ + regval = (regval>> 6) * asc7621_in_scaling[nr] / (0xc0<< 2); +Looks much better :)return sprintf(buf, "%u\n", regval); } @@ -331,7 +328,7 @@ static ssize_t show_in8(struct device *dev, struct device_attribute *attr, return sprintf(buf, "%u\n", ((data->reg[param->msb[0]] * - asc7621_in_scaling[nr]) / 256)); + asc7621_in_scaling[nr]) / 0xc0)); } static ssize_t store_in8(struct device *dev, struct device_attribute *attr, @@ -344,9 +341,11 @@ static ssize_t store_in8(struct device *dev, struct device_attribute *attr, if (strict_strtol(buf, 10,&reqval)) return -EINVAL; - reqval = SENSORS_LIMIT(reqval, 0, asc7621_in_scaling[nr]); + reqval = SENSORS_LIMIT(reqval, 0, 0xffff);Do you really need this? There's another call to SENSORS_LIMIT() below, I thought it would be enough.+ + reqval = reqval * 0xc0 / asc7621_in_scaling[nr]; - reqval = (reqval * 255 + 128) / asc7621_in_scaling[nr]; + reqval = SENSORS_LIMIT(reqval, 0, 0xff); mutex_lock(&data->update_lock); data->reg[param->msb[0]] = reqval; @@ -846,11 +845,11 @@ static struct asc7621_param asc7621_params[] = { PWRITE(in3_max, 3, PRI_LOW, 0x4b, 0, 0, 0, in8), PWRITE(in4_max, 4, PRI_LOW, 0x4d, 0, 0, 0, in8), - PREAD(in0_alarm, 0, PRI_LOW, 0x41, 0, 0x01, 0, bitmask), - PREAD(in1_alarm, 1, PRI_LOW, 0x41, 0, 0x01, 1, bitmask), - PREAD(in2_alarm, 2, PRI_LOW, 0x41, 0, 0x01, 2, bitmask), - PREAD(in3_alarm, 3, PRI_LOW, 0x41, 0, 0x01, 3, bitmask), - PREAD(in4_alarm, 4, PRI_LOW, 0x42, 0, 0x01, 0, bitmask), + PREAD(in0_alarm, 0, PRI_HIGH, 0x41, 0, 0x01, 0, bitmask), + PREAD(in1_alarm, 1, PRI_HIGH, 0x41, 0, 0x01, 1, bitmask), + PREAD(in2_alarm, 2, PRI_HIGH, 0x41, 0, 0x01, 2, bitmask), + PREAD(in3_alarm, 3, PRI_HIGH, 0x41, 0, 0x01, 3, bitmask), + PREAD(in4_alarm, 4, PRI_HIGH, 0x42, 0, 0x01, 0, bitmask), PREAD(fan1_input, 0, PRI_HIGH, 0x29, 0x28, 0, 0, fan16), PREAD(fan2_input, 1, PRI_HIGH, 0x2b, 0x2a, 0, 0, fan16), @@ -862,10 +861,10 @@ static struct asc7621_param asc7621_params[] = { PWRITE(fan3_min, 2, PRI_LOW, 0x59, 0x58, 0, 0, fan16), PWRITE(fan4_min, 3, PRI_LOW, 0x5b, 0x5a, 0, 0, fan16), - PREAD(fan1_alarm, 0, PRI_LOW, 0x42, 0, 0x01, 0, bitmask), - PREAD(fan2_alarm, 1, PRI_LOW, 0x42, 0, 0x01, 1, bitmask), - PREAD(fan3_alarm, 2, PRI_LOW, 0x42, 0, 0x01, 2, bitmask), - PREAD(fan4_alarm, 3, PRI_LOW, 0x42, 0, 0x01, 3, bitmask), + PREAD(fan1_alarm, 0, PRI_HIGH, 0x42, 0, 0x01, 2, bitmask), + PREAD(fan2_alarm, 1, PRI_HIGH, 0x42, 0, 0x01, 3, bitmask), + PREAD(fan3_alarm, 2, PRI_HIGH, 0x42, 0, 0x01, 4, bitmask), + PREAD(fan4_alarm, 3, PRI_HIGH, 0x42, 0, 0x01, 5, bitmask), PREAD(temp1_input, 0, PRI_HIGH, 0x25, 0x10, 0, 0, temp10), PREAD(temp2_input, 1, PRI_HIGH, 0x26, 0x15, 0, 0, temp10), @@ -886,10 +885,10 @@ static struct asc7621_param asc7621_params[] = { PWRITE(temp3_max, 2, PRI_LOW, 0x53, 0, 0, 0, temp8), PWRITE(temp4_max, 3, PRI_LOW, 0x35, 0, 0, 0, temp8), - PREAD(temp1_alarm, 0, PRI_LOW, 0x41, 0, 0x01, 4, bitmask), - PREAD(temp2_alarm, 1, PRI_LOW, 0x41, 0, 0x01, 5, bitmask), - PREAD(temp3_alarm, 2, PRI_LOW, 0x41, 0, 0x01, 6, bitmask), - PREAD(temp4_alarm, 3, PRI_LOW, 0x43, 0, 0x01, 0, bitmask), + PREAD(temp1_alarm, 0, PRI_HIGH, 0x41, 0, 0x01, 4, bitmask), + PREAD(temp2_alarm, 1, PRI_HIGH, 0x41, 0, 0x01, 5, bitmask), + PREAD(temp3_alarm, 2, PRI_HIGH, 0x41, 0, 0x01, 6, bitmask), + PREAD(temp4_alarm, 3, PRI_HIGH, 0x43, 0, 0x01, 0, bitmask), PWRITE(temp1_source, 0, PRI_LOW, 0x02, 0, 0x07, 4, bitmask), PWRITE(temp2_source, 1, PRI_LOW, 0x02, 0, 0x07, 0, bitmask), @@ -898,7 +897,7 @@ static struct asc7621_param asc7621_params[] = { PWRITE(temp1_smoothing_enable, 0, PRI_LOW, 0x62, 0, 0x01, 3, bitmask), PWRITE(temp2_smoothing_enable, 1, PRI_LOW, 0x63, 0, 0x01, 7, bitmask), - PWRITE(temp3_smoothing_enable, 2, PRI_LOW, 0x64, 0, 0x01, 3, bitmask), + PWRITE(temp3_smoothing_enable, 2, PRI_LOW, 0x63, 0, 0x01, 3, bitmask), PWRITE(temp4_smoothing_enable, 3, PRI_LOW, 0x3c, 0, 0x01, 3, bitmask), PWRITE(temp1_smoothing_time, 0, PRI_LOW, 0x62, 0, 0x07, 0, temp_st), -- 1.7.1
_______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
