Jean Delvare wrote: > Hi Hans, > > On Mon, 08 Oct 2007 13:08:08 +0200, Hans de Goede wrote: >> This patch adds a new merged driver for FSC sensor chips, it merges the fscher >> and fscpos drivers and adds support for the FSC Scylla, Heracles and Heimdall >> chips. >> >> This version of the patch has had all issues found during the review by Jean >> Delvare addressed: >> http://lists.lm-sensors.org/pipermail/lm-sensors/2007-October/021396.html >> >> Signed-off-by: Hans de Goede <j.w.r.degoede at hhs.nl> >> >> Jean, can you please ack this one? > > Can't ack it as there's one bug remaining :/ And I have a few > suggestions for improvements as well. > >> +static ssize_t store_alert_led(struct device *dev, >> + struct device_attribute *devattr, const char *buf, size_t count) >> +{ >> + u8 reg; >> + struct fschmd_data *data = dev_get_drvdata(dev); >> + unsigned long v = simple_strtoul(buf, NULL, 10); >> + >> + mutex_lock(&data->update_lock); >> + >> + reg = i2c_smbus_read_byte_data(&data->client, FSCHMD_REG_CONTROL); >> + >> + if (v) >> + reg |= FSCHMD_CONTROL_ALERT_LED_MASK; >> + else >> + reg &= ~FSCHMD_CONTROL_ALERT_LED_MASK; >> + >> + i2c_smbus_write_byte_data(&data->client, FSCHMD_REG_CONTROL, v); > > You want to write "reg" here, not "v". > Good catch (caused by the do not use update_device changes). >> + >> + data->global_control = reg; >> + >> + mutex_unlock(&data->update_lock); >> + >> + return count; >> +} >> + > > You could also use FSCHMD_CONTROL_ALERT_LED_MASK instead of 0x01 in > show_alert_led(). > Will fix. >> +static int fschmd_detect(struct i2c_adapter *adapter, int address, int kind) >> +{ >> (...) >> + for (i = 0; i < ARRAY_SIZE(fschmd_attr); i++) { >> + err = device_create_file(&client->dev, >> + &fschmd_attr[i].dev_attr); >> + if (err) >> + goto exit_detach; >> + } >> + >> + for (i = 0; i < (FSCHMD_NO_TEMP_SENSORS[data->kind] * 4); i++) { > > Why don't you just use ARRAY_SIZE()? Less likely to break with future > changes. > Because the array contains attributes for the maximum number of temp sensors and not all models have the maximum number, hence FSCHMD_NO_TEMP_SENSORS[data->kind] gets used, I agree this isn't ideal. >> + /* Poseidon doesn't have TEMP_LIMIT registers */ >> + if (kind == fscpos && fschmd_temp_attr[i].dev_attr.show == >> + show_temp_max) >> + continue; >> + >> + err = device_create_file(&client->dev, >> + &fschmd_temp_attr[i].dev_attr); >> + if (err) >> + goto exit_detach; >> + } >> + >> + for (i = 0; i < (FSCHMD_NO_FAN_SENSORS[data->kind] * 5); i++) { > > Same here. And same in fschmd_detach_client(). > See above. > The rest looks all OK now. Please post a last update and I'll ack it > tomorrow. > I'll do a new version somewhere today. > One more note: there's a lot of trailing whitespace in your driver. I > think Mark will strip it before committing your patch but it would be > easier for everyone if you could avoid trailing whitespace in future > patches. > Are there any tools for this? You've mentioned this before and I have tried to avoid adding trailing whitespace while editing, but I guess that doesn't help much. Regards, Hans
