Hi Guenter,
On Sun, 20 Apr 2014 11:41:17 -0700, Guenter Roeck wrote:
> Function macros make the code harder to read and increase code size,
> so drop them.
>
> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> ---
> v3: No comma after t_num_entries.
> Make temp_regs const and provide array size.
I see the const but not the array size, see below.
> Fix show_temp_hyst, specifically display of low limit hysteresis.
> Simplify set_temp and handling of hysteresis.
I'm afraid you introduced a regression with that change, see below.
> --- a/drivers/hwmon/lm77.c
> +++ b/drivers/hwmon/lm77.c
> (...)
> +static const u8 temp_regs[] = {
No t_num_temp?
> + [t_input] = LM77_REG_TEMP,
> + [t_min] = LM77_REG_TEMP_MIN,
> + [t_max] = LM77_REG_TEMP_MAX,
> + [t_crit] = LM77_REG_TEMP_CRIT,
> + [t_hyst] = LM77_REG_TEMP_HYST,
> +};
> +
> /* Each client has this additional data */
> struct lm77_data {
> struct device *hwmon_dev;
> struct mutex update_lock;
> char valid;
> unsigned long last_updated; /* In jiffies */
> - int temp_input; /* Temperatures */
> - int temp_crit;
> - int temp_min;
> - int temp_max;
> - int temp_hyst;
> + int temp[t_num_temp];/* index using temp_index */
A space between ; and /* would improve readability IMHO.
> u8 alarms;
> };
> (...)
> +static ssize_t set_temp(struct device *dev, struct device_attribute *devattr,
> + const char *buf, size_t count)
> {
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> struct i2c_client *client = to_i2c_client(dev);
> struct lm77_data *data = i2c_get_clientdata(client);
> - unsigned long val;
> - int err;
> + int nr = attr->index;
> + int err, oldhyst;
> + long val;
>
> - err = kstrtoul(buf, 10, &val);
> + err = kstrtol(buf, 10, &val);
> if (err)
> return err;
>
> mutex_lock(&data->update_lock);
> - data->temp_hyst = data->temp_crit - val;
> - lm77_write_value(client, LM77_REG_TEMP_HYST,
> - LM77_TEMP_TO_REG(data->temp_hyst));
> + if (nr == t_crit) {
> + /* preserve hysteresis when setting T_crit */
> + oldhyst = data->temp[nr] - data->temp[t_hyst];
> + data->temp[t_hyst] = data->temp[nr] - oldhyst;
The above pair of lines is a no-op. You want to use val (the new value)
in the second line, instead of data->temp[nr] (the old value.)
That being said, if you are going to remove this feature in the next
patch, you might as well swap the patches and remove it first. That way
you won't have to deal with it here.
> + lm77_write_value(client, LM77_REG_TEMP_HYST,
> + LM77_TEMP_TO_REG(data->temp[t_hyst]));
> + }
> + data->temp[nr] = val;
> + lm77_write_value(client, temp_regs[nr], LM77_TEMP_TO_REG(val));
> mutex_unlock(&data->update_lock);
> return count;
> }
--
Jean Delvare
SUSE L3 Support
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
