Re: [lm-sensors] PATCH: hwmon-fschmd-new-driver-v2.patch

[Hans de Goede <j.w.r.degoede@hhs.nl>]

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@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

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors