On Wed, Dec 01, 2010 at 02:59:06PM +0100, Jean Delvare wrote:
> Your patch is correct. However, in order to get the fix in stable
> kernel series, I would prefer an even more minimalist fix, only
> addressing the actual bug. The two minor optimizations (skipping the
> register write if the value is the same and skipping the call to
> fixup_fan_min if the value is different but resulted in the same
> register encoding) would rather be left out at this time.
>
> This would result in the following patch:
>
> ---
> drivers/hwmon/adm1026.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> --- linux-2.6.37-rc4.orig/drivers/hwmon/adm1026.c 2010-12-01 13:48:13.000000000 +0100
> +++ linux-2.6.37-rc4/drivers/hwmon/adm1026.c 2010-12-01 14:20:14.000000000 +0100
> @@ -916,7 +916,7 @@ static ssize_t set_fan_div(struct device
> int nr = sensor_attr->index;
> struct i2c_client *client = to_i2c_client(dev);
> struct adm1026_data *data = i2c_get_clientdata(client);
> - int val, orig_div, new_div, shift;
> + int val, orig_div, new_div;
>
> val = simple_strtol(buf, NULL, 10);
> new_div = DIV_TO_REG(val);
> @@ -928,15 +928,17 @@ static ssize_t set_fan_div(struct device
> data->fan_div[nr] = DIV_FROM_REG(new_div);
>
> if (nr < 4) { /* 0 <= nr < 4 */
> - shift = 2 * nr;
> adm1026_write_value(client, ADM1026_REG_FAN_DIV_0_3,
> - ((DIV_TO_REG(orig_div) & (~(0x03 << shift))) |
> - (new_div << shift)));
> + (DIV_TO_REG(data->fan_div[0]) << 0) |
> + (DIV_TO_REG(data->fan_div[1]) << 2) |
> + (DIV_TO_REG(data->fan_div[2]) << 4) |
> + (DIV_TO_REG(data->fan_div[3]) << 6));
> } else { /* 3 < nr < 8 */
> - shift = 2 * (nr - 4);
> adm1026_write_value(client, ADM1026_REG_FAN_DIV_4_7,
> - ((DIV_TO_REG(orig_div) & (~(0x03 << (2 * shift)))) |
> - (new_div << shift)));
> + (DIV_TO_REG(data->fan_div[4]) << 0) |
> + (DIV_TO_REG(data->fan_div[5]) << 2) |
> + (DIV_TO_REG(data->fan_div[6]) << 4) |
> + (DIV_TO_REG(data->fan_div[7]) << 6));
> }
>
> if (data->fan_div[nr] != orig_div) {
>
> As you can see, it's more compact and immediately obviously correct. So
> backporting it to kernels 2.6.36, 2.6.32, 2.6.27 and maybe even
> 2.6.16 will be trivial. Is this OK with you? You retain patch
> authorship, of course.
Sounds good to me. Do I need to submit it again, or you will take care
of the re-submission?
> Regarding the optimizations, I'm not even sure if we need them.
We could debate this issues for weeks but I am sure we all have better
things to do than trying to overoptimize a non critical code path. :-)
thanks,
GG
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
