Hi Jean, Again, revised patch attached. The array overflow in the fan configuration was upsetting the fan readings, which is now fixed. The alarm flags from the device are not what as expected, so I have added code to explicitly correct the alarm behaviour. The alarm is now set if the fan stalls and a minimum RPM has been set. The alarm is explicitly cleared if the fan is not spinning and no minimum RPM has been set. These problems were masked by the fan array overflow you spotted. I have updated the temperature measurement for temp2-6, but I have no way of testing this code as I have no hardware that matches it. Hopefully someone can test it and report back. It also includes your performance optimisations for the fan LSB calculations, but again I have no way to test temp2-6 for this. I hope I have got the header information and patch format correct now - I had been using an older version of Quilt (not sure which as I could not find any way to get Quilt to report its version number) and this included the trailing whitespace removal, so hopefully this is OK too. Finally, I have added my contact details to the MAINTAINERS list. Best regards, Roger > -----Original Message----- > From: Jean Delvare [mailto:khali at linux-fr.org] > Sent: 16 November 2005 17:07 > To: roger at planbit.co.uk > Cc: Mark Studebaker; LM Sensors > Subject: Re: VT8231 patch for review > > > Hi Roger, > > On 2005-11-16, Roger Lucas wrote: > > Firstly, I really like Quilt. It makes the whole patch process much > > easier. > > True :) I'd suggest that you add the following to your ~/.quiltrc: > > QUILT_NO_DIFF_INDEX=1 > QUILT_REFRESH_ARGS="--diffstat --strip-trailing-whitespace" > > This will help you produce patches the kernel folks will enjoy. > > > To answer the question in your last mail, "Yes, I am happy to be the > > maintainer of this driver". > > Great. Then please submit a patch adding yourself to MAINTAINERS. Copy my > LM90 entry for example, and adapt to you and the VT8231. Pay attention > to the fact that the entries are alphabetically sorted. > > Note that all your patches should include a "Signed-off-by" line. This > one lacked it. > > > Once we go around the loop to get this into the kernel, I'll take a look > > at the user-space utility as it throws up a load of errors at the > moment. > > It also seems to get the alarm wrong on the fans. > > Please look into it *before* the driver is accepted into the Linux 2.6 > kernel tree. Maybe the bugs are in the user-space part, but maybe not. > We better don't merge a bogus driver. > > There's still one thing I am not happy with: temp 2-6. We agreed to > export the raw register value for the diode-based temperature (temp1) > because there's nothing better we can do, but for the thermistor-based > ones we should be able to export the value as a voltage. > > The BIOS porting guide states that the register value is equal to 253 - > R6/(R6+R5) * 210, where R5 is a thermisor (10K at 25C) and R6 is a 10K > resistor. These 253 and 210 constants are inherent to the chip and do > not depend on the motherboard. R6 and R5 form a standard thermistor > divider, effectively converting a temperature into a voltage. > > So I believe we should have the following for temp 2-6: > > TEMP_FROM_REG(reg) ((253 - (reg)) * 1000 / 210) > > TEMP_TO_REG(val) (253 - (val) * 210 / 1000) > > Of course we would then need to update the formula in sensors.conf.eg. > > To the code itself now: > > > Copyright (c) 2005 Roger Lucas <roger at planbit.co.uk> > > 2002 Mark D. Studebaker <mdsxyz123 at yahoo.com> > > I think you have to repeat "Copyright (c)" before "2002". > > > #define VT8231_REG_TEMP_LOW01 0x49 > > #define VT8231_REG_TEMP_LOW25 0x4d > > (...) > > ** All the on-chip hardware temperature comparisons for the alarms > > are only > > ** 8-bits wide, and compare against the 8 MSBs of the temperature. > > The bits > > ** in the registers VT8231_REG_TEMP_LOW12 and VT8231_REG_TEMP_LOW36 > > are > > ** ignored. > > Conflicting conventions. > > > u8 fan[2]; /* Register value */ > > u8 fan_min[2]; /* Register value */ > > (...) > > for (i = 1; i < 3; i++) { > > data->fan[i - 1] = vt8231_read_value(client, > > VT8231_REG_FAN(i)); > > data->fan_min[i - 1] = vt8231_read_value(client, > > VT8231_REG_FAN_MIN(i)); > > } > > Array overflow! Sorry for not noticing before. > > > static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, show_in5, NULL, 5); > > static SENSOR_DEVICE_ATTR(in5_min, S_IRUGO | S_IWUSR, show_in5_min, > > set_in5_min, 5); > > static SENSOR_DEVICE_ATTR(in5_max, S_IRUGO | S_IWUSR, show_in5_max, > > set_in5_max, 5); > > Please use DEVICE_ATTR instead. You are not using the extra parameter in > the callback functions. > > > /* Temperatures */ > > static ssize_t show_temp(struct device *dev, struct device_attribute > *attr, > > char *buf) > > { > > struct sensor_device_attribute *sensor_attr = > > to_sensor_dev_attr(attr); > > int nr = sensor_attr->index; > > struct vt8231_data *data = vt8231_update_device(dev); > > return sprintf(buf, "%d\n", data->temp[nr] * 250); > > } > > Double space on last line. > > > /* NOTE THAT THESE MAP THE LINUX TEMPERATURE SENSOR NUMBERING (1-6) TO > > THE VIA > > ** TEMPERATURE SENSOR NUMBERING (0-5) > > */ > > No caps please. Yelling never makes you more right. > > > #define CFG_INFO_TEMP(id) { id - 1, \ > > &sensor_dev_attr_temp##id##_input.dev_attr, \ > > &sensor_dev_attr_temp##id##_min.dev_attr, \ > > &sensor_dev_attr_temp##id##_max.dev_attr } > > #define CFG_INFO_VOLT(id) { id, \ > > &sensor_dev_attr_in##id##_input.dev_attr, \ > > &sensor_dev_attr_in##id##_min.dev_attr, \ > > &sensor_dev_attr_in##id##_max.dev_attr, \ > > } > > > > struct str_device_attr_table { > > int id; > > struct device_attribute *input; > > struct device_attribute *min; > > struct device_attribute *max; > > }; > > > > static struct str_device_attr_table cfg_info_temp[] = { > > CFG_INFO_TEMP(1), > > CFG_INFO_TEMP(2), > > CFG_INFO_TEMP(3), > > CFG_INFO_TEMP(4), > > CFG_INFO_TEMP(5), > > CFG_INFO_TEMP(6), > > { .id = -1 } > > }; > > > > static struct str_device_attr_table cfg_info_volt[] = { > > CFG_INFO_VOLT(0), > > CFG_INFO_VOLT(1), > > CFG_INFO_VOLT(2), > > CFG_INFO_VOLT(3), > > CFG_INFO_VOLT(4), > > CFG_INFO_VOLT(5), > > { .id = -1 } > > }; > > You can make these tables smaller. First, use the ARRAY_SIZE macro > instead of a list terminator. Second, you can easily deduce the value of > .id from the position in the array (in fact, .id *is* the position in > the array) so you don't need this field. > > > static ssize_t show_fan(struct device *dev, struct device_attribute > *attr, > > char *buf) > > { > > struct sensor_device_attribute *sensor_attr = > > to_sensor_dev_attr(attr); > > int nr = sensor_attr->index - 1; > > That "- 1" should no more be there, right? > > > vt8231_write_value(client, VT8231_REG_FAN_MIN(nr+1), > > data->fan_min[nr]); > > I think I said it before, but why don't you change the definition of > VT8231_REG_FAN and VT8231_REG_FAN_MIN to: > > #define VT8231_REG_FAN_MIN(nr) (0x3b + (nr)) > #define VT8231_REG_FAN(nr) (0x29 + (nr)) > > And then call them with 0..1 rather than 1..2? This would make the code > more simple and easier to check against the datasheet. > > > /* Correct the fan minimum speed */ > > data->fan_min[nr] = FAN_TO_REG(min, DIV_FROM_REG(data- > >fan_div[nr])); > > You additionally need to write the value to the chip, else it will be > overwritten on next update. > > > int vt8231_detect(struct i2c_adapter *adapter) > > { > > struct i2c_client *new_client; > > BTW, feel free to name it simply "client" if you want to. This "new_" > prefix really doesn't add any value. > > > /* Fill in the remaining client fields and put into the global > > list */ > > strcpy(new_client->name, "vt8231"); > > Ideally this should be strlcpy(new_client->name, "vt8231", > I2C_NAME_SIZE); > > > for (i = 0; i < 6; i++) { > > if (ISTEMP(i, data->uch_config)) { > > data->temp[i] = vt8231_read_value(client, > > regtemp[i]) << 2; > > switch(i) { > > case 0: j = (vt8231_read_value(client, > > VT8231_REG_TEMP_LOW01) > > >> 6) & 0x03; > > break; > > > > case 1: j = (vt8231_read_value(client, > > VT8231_REG_TEMP_LOW01) > > >> 4) & 0x03; > > break; > > case 2: > > case 3: > > case 4: > > case 5: j = (vt8231_read_value(client, > > VT8231_REG_TEMP_LOW25) > > >> ((i-2)*2)) & 0x03; > > break; > > > > default: j = 0; > > break; > > } > > data->temp[i] |= j; > > (...) > > } > > } > > I just realized how inefficient this part is. What about the following > instead: > > u16 low = vt8231_read_value(client, VT8231_REG_TEMP_LOW01); > low = (low >> 6) | ((low & 0x30) >> 2); /* swap and shift */ > | (vt8231_read_value(client, VT8231_REG_TEMP_LOW25) << 4); > for (i = 0; i < 6; i++) { > if (ISTEMP(i, data->uch_config)) { > data->temp[i] = (vt8231_read_value(client, regtemp[i]) << 2) > | ((low >> 2*i) & 0x03); > (...) > } > } > > (You can even drop j if you do that.) > > > module_init(sm_vt8231_init); > > module_exit(sm_vt8231_exit); > > > > > > No blank lines at end of file please ;) > > Thanks a lot for your good work! Keep it up, we're getting closer :) > > -- > Jean Delvare -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: vt8231.patch.r6.txt Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20051116/7bae48f0/attachment.txt
