* Re: [lm-sensors] [patch 1/1] w83627hf hoist nr-1 offset out of
2007-10-12 22:50 [lm-sensors] [patch 1/1] w83627hf hoist nr-1 offset out of Jim Cromie
@ 2007-10-12 23:56 ` Jim Cromie
2007-10-14 8:36 ` Jean Delvare
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jim Cromie @ 2007-10-12 23:56 UTC (permalink / raw)
To: lm-sensors
[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]
this patch replaces last, fixing (long) cast placement,
and one previously missed (nr>=2) -> (nr)
This hoists nr-1 offset out of (show|store)_temp_*(.*) callbacks, and into
SENSOR_DEVICE_ATTRs for sysfs tempN_X files. It also combines
temp[1] and temp_add[2] (array) fields in w83627hf_data into 3 elem arrays,
which simplifies special-case handling of nr, allowing simplification
of callback bodies and rerolling a flattened loop in
w83627hf_update_device(struct device *dev).
The array conversion changes temp[1] from u8 to u16, but this was
happening implicitly via the helper functions anyway.
Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
$ diffstat diff.hwmon-w83627hf-hoist-temp-offsets
drivers/hwmon/w83627hf.c | 120 +++++++++++++++++------------------------------
1 files changed, 44 insertions(+), 76 deletions(-)
> size shrinks too:
> 12982 2652 36 15670 3d36 hwmon-demacro/drivers/hwmon/w83627hf.ko
> 12850 2652 36 15538 3cb2 hwmon-hf-2/drivers/hwmon/w83627hf.ko
>
> and the u8->u16 doesnt seem to cost anything in the data section. (padding?)
>
>
[-- Attachment #2: diff.hwmon-w83627hf-hoist-temp-offsets --]
[-- Type: application/octet-stream, Size: 7134 bytes --]
Binary files hwmon-demacro/arch/i386/boot/setup.elf and hwmon-hf-1/arch/i386/boot/setup.elf differ
diff -ruNp -X dontdiff -X exclude-diffs hwmon-demacro/drivers/hwmon/w83627hf.c hwmon-hf-1/drivers/hwmon/w83627hf.c
--- hwmon-demacro/drivers/hwmon/w83627hf.c 2007-10-11 16:03:10.000000000 -0600
+++ hwmon-hf-1/drivers/hwmon/w83627hf.c 2007-10-12 17:42:11.000000000 -0600
@@ -175,15 +175,10 @@ superio_exit(void)
#define W83781D_REG_TEMP2_CONFIG 0x152
#define W83781D_REG_TEMP3_CONFIG 0x252
-#define W83781D_REG_TEMP(nr) ((nr == 3) ? (0x0250) : \
- ((nr == 2) ? (0x0150) : \
- (0x27)))
-#define W83781D_REG_TEMP_HYST(nr) ((nr == 3) ? (0x253) : \
- ((nr == 2) ? (0x153) : \
- (0x3A)))
-#define W83781D_REG_TEMP_OVER(nr) ((nr == 3) ? (0x255) : \
- ((nr == 2) ? (0x155) : \
- (0x39)))
+/* these are zero-based, unlike config constants above */
+static u16 w83781d_reg_temp[] = { 0x27, 0x150, 0x250 };
+static u16 w83781d_reg_temp_hyst[] = { 0x3A, 0x153, 0x253 };
+static u16 w83781d_reg_temp_over[] = { 0x39, 0x155, 0x255 };
#define W83781D_REG_BANK 0x4E
@@ -360,12 +355,9 @@ struct w83627hf_data {
u8 in_min[9]; /* Register value */
u8 fan[3]; /* Register value */
u8 fan_min[3]; /* Register value */
- u8 temp;
- u8 temp_max; /* Register value */
- u8 temp_max_hyst; /* Register value */
- u16 temp_add[2]; /* Register value */
- u16 temp_max_add[2]; /* Register value */
- u16 temp_max_hyst_add[2]; /* Register value */
+ u16 temp[3];
+ u16 temp_max[3]; /* Register value */
+ u16 temp_max_hyst[3]; /* Register value */
u8 fan_div[3]; /* Register encoding, shifted right */
u8 vid; /* Register encoding, combined */
u32 alarms; /* Register encoding, combined */
@@ -610,12 +602,10 @@ show_temp(struct device *dev, struct dev
{
int nr = to_sensor_dev_attr(devattr)->index;
struct w83627hf_data *data = w83627hf_update_device(dev);
- if (nr >= 2) { /* TEMP2 and TEMP3 */
- return sprintf(buf, "%ld\n",
- (long)LM75_TEMP_FROM_REG(data->temp_add[nr-2]));
- } else { /* TEMP1 */
- return sprintf(buf, "%ld\n", (long)TEMP_FROM_REG(data->temp));
- }
+
+ u16 tmp = data->temp[nr];
+ return sprintf(buf, "%ld\n", (nr) ? (long) LM75_TEMP_FROM_REG(tmp)
+ : (long) TEMP_FROM_REG(tmp));
}
static ssize_t
@@ -624,13 +614,10 @@ show_temp_max(struct device *dev, struct
{
int nr = to_sensor_dev_attr(devattr)->index;
struct w83627hf_data *data = w83627hf_update_device(dev);
- if (nr >= 2) { /* TEMP2 and TEMP3 */
- return sprintf(buf, "%ld\n",
- (long)LM75_TEMP_FROM_REG(data->temp_max_add[nr-2]));
- } else { /* TEMP1 */
- return sprintf(buf, "%ld\n",
- (long)TEMP_FROM_REG(data->temp_max));
- }
+
+ u16 tmp = data->temp_max[nr];
+ return sprintf(buf, "%ld\n", (nr) ? (long) LM75_TEMP_FROM_REG(tmp)
+ : (long) TEMP_FROM_REG(tmp));
}
static ssize_t
@@ -639,13 +626,10 @@ show_temp_max_hyst(struct device *dev, s
{
int nr = to_sensor_dev_attr(devattr)->index;
struct w83627hf_data *data = w83627hf_update_device(dev);
- if (nr >= 2) { /* TEMP2 and TEMP3 */
- return sprintf(buf, "%ld\n",
- (long)LM75_TEMP_FROM_REG(data->temp_max_hyst_add[nr-2]));
- } else { /* TEMP1 */
- return sprintf(buf, "%ld\n",
- (long)TEMP_FROM_REG(data->temp_max_hyst));
- }
+
+ u16 tmp = data->temp_max_hyst[nr];
+ return sprintf(buf, "%ld\n", (nr) ? (long) LM75_TEMP_FROM_REG(tmp)
+ : (long) TEMP_FROM_REG(tmp));
}
static ssize_t
@@ -655,18 +639,16 @@ store_temp_max(struct device *dev, struc
int nr = to_sensor_dev_attr(devattr)->index;
struct w83627hf_data *data = dev_get_drvdata(dev);
long val = simple_strtol(buf, NULL, 10);
+ u16 tmp;
mutex_lock(&data->update_lock);
- if (nr >= 2) { /* TEMP2 and TEMP3 */
- data->temp_max_add[nr-2] = LM75_TEMP_TO_REG(val);
- w83627hf_write_value(data, W83781D_REG_TEMP_OVER(nr),
- data->temp_max_add[nr-2]);
- } else { /* TEMP1 */
- data->temp_max = TEMP_TO_REG(val);
- w83627hf_write_value(data, W83781D_REG_TEMP_OVER(nr),
- data->temp_max);
- }
+ tmp = (nr) ? LM75_TEMP_TO_REG(val)
+ : TEMP_TO_REG(val);
+
+ data->temp_max[nr] = tmp;
+ w83627hf_write_value(data, w83781d_reg_temp_over[nr], tmp);
+
mutex_unlock(&data->update_lock);
return count;
}
@@ -678,29 +660,27 @@ store_temp_max_hyst(struct device *dev,
int nr = to_sensor_dev_attr(devattr)->index;
struct w83627hf_data *data = dev_get_drvdata(dev);
long val = simple_strtol(buf, NULL, 10);
+ u16 tmp;
mutex_lock(&data->update_lock);
- if (nr >= 2) { /* TEMP2 and TEMP3 */
- data->temp_max_hyst_add[nr-2] = LM75_TEMP_TO_REG(val);
- w83627hf_write_value(data, W83781D_REG_TEMP_HYST(nr),
- data->temp_max_hyst_add[nr-2]);
- } else { /* TEMP1 */
- data->temp_max_hyst = TEMP_TO_REG(val);
- w83627hf_write_value(data, W83781D_REG_TEMP_HYST(nr),
- data->temp_max_hyst);
- }
+ tmp = (nr) ? LM75_TEMP_TO_REG(val)
+ : TEMP_TO_REG(val);
+
+ data->temp_max_hyst[nr] = tmp;
+ w83627hf_write_value(data, w83781d_reg_temp_hyst[nr], tmp);
+
mutex_unlock(&data->update_lock);
return count;
}
#define sysfs_temp_decl(offset) \
static SENSOR_DEVICE_ATTR(temp##offset##_input, S_IRUGO, \
- show_temp, NULL, offset); \
+ show_temp, NULL, offset - 1); \
static SENSOR_DEVICE_ATTR(temp##offset##_max, S_IRUGO|S_IWUSR, \
- show_temp_max, store_temp_max, offset); \
+ show_temp_max, store_temp_max, offset - 1); \
static SENSOR_DEVICE_ATTR(temp##offset##_max_hyst, S_IRUGO|S_IWUSR, \
- show_temp_max_hyst, store_temp_max_hyst, offset);
+ show_temp_max_hyst, store_temp_max_hyst, offset - 1);
sysfs_temp_decl(1);
sysfs_temp_decl(2);
@@ -1543,7 +1523,7 @@ static void __devinit w83627hf_init_devi
static struct w83627hf_data *w83627hf_update_device(struct device *dev)
{
struct w83627hf_data *data = dev_get_drvdata(dev);
- int i;
+ int i, num_temps = (data->type == w83697hf) ? 2 : 3;
mutex_lock(&data->update_lock);
@@ -1596,25 +1576,13 @@ static struct w83627hf_data *w83627hf_up
break;
}
}
-
- data->temp = w83627hf_read_value(data, W83781D_REG_TEMP(1));
- data->temp_max =
- w83627hf_read_value(data, W83781D_REG_TEMP_OVER(1));
- data->temp_max_hyst =
- w83627hf_read_value(data, W83781D_REG_TEMP_HYST(1));
- data->temp_add[0] =
- w83627hf_read_value(data, W83781D_REG_TEMP(2));
- data->temp_max_add[0] =
- w83627hf_read_value(data, W83781D_REG_TEMP_OVER(2));
- data->temp_max_hyst_add[0] =
- w83627hf_read_value(data, W83781D_REG_TEMP_HYST(2));
- if (data->type != w83697hf) {
- data->temp_add[1] =
- w83627hf_read_value(data, W83781D_REG_TEMP(3));
- data->temp_max_add[1] =
- w83627hf_read_value(data, W83781D_REG_TEMP_OVER(3));
- data->temp_max_hyst_add[1] =
- w83627hf_read_value(data, W83781D_REG_TEMP_HYST(3));
+ for (i=0; i<num_temps; i++) {
+ data->temp[i] = w83627hf_read_value(
+ data, w83781d_reg_temp[i]);
+ data->temp_max[i] = w83627hf_read_value(
+ data, w83781d_reg_temp_over[i]);
+ data->temp_max_hyst[i] = w83627hf_read_value(
+ data, w83781d_reg_temp_hyst[i]);
}
i = w83627hf_read_value(data, W83781D_REG_VID_FANDIV);
[-- Attachment #3: Type: text/plain, Size: 153 bytes --]
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [patch 1/1] w83627hf hoist nr-1 offset out of
2007-10-12 22:50 [lm-sensors] [patch 1/1] w83627hf hoist nr-1 offset out of Jim Cromie
2007-10-12 23:56 ` Jim Cromie
@ 2007-10-14 8:36 ` Jean Delvare
2007-10-14 23:10 ` Jim Cromie
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2007-10-14 8:36 UTC (permalink / raw)
To: lm-sensors
Hi Jim,
On Fri, 12 Oct 2007 17:56:41 -0600, Jim Cromie wrote:
> this patch replaces last, fixing (long) cast placement,
> and one previously missed (nr>=2) -> (nr)
>
> This hoists nr-1 offset out of (show|store)_temp_*(.*) callbacks, and into
> SENSOR_DEVICE_ATTRs for sysfs tempN_X files. It also combines
> temp[1] and temp_add[2] (array) fields in w83627hf_data into 3 elem arrays,
> which simplifies special-case handling of nr, allowing simplification
> of callback bodies and rerolling a flattened loop in
> w83627hf_update_device(struct device *dev).
Very nice cleanup, thanks.
> The array conversion changes temp[1] from u8 to u16, but this was
> happening implicitly via the helper functions anyway.
>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
> $ diffstat diff.hwmon-w83627hf-hoist-temp-offsets
> drivers/hwmon/w83627hf.c | 120 +++++++++++++++++------------------------------
> 1 files changed, 44 insertions(+), 76 deletions(-)
>
>
> > size shrinks too:
> > 12982 2652 36 15670 3d36 hwmon-demacro/drivers/hwmon/w83627hf.ko
> > 12850 2652 36 15538 3cb2 hwmon-hf-2/drivers/hwmon/w83627hf.ko
> >
> > and the u8->u16 doesnt seem to cost anything in the data section. (padding?)
The struct w83627hf_data is allocated dynamically so it doesn't appear
in any section of the binary. The 3 static reg_temp arrays should show
in the data section... not sure why they don't.
Review:
> --- hwmon-demacro/drivers/hwmon/w83627hf.c 2007-10-11 16:03:10.000000000 -0600
> +++ hwmon-hf-1/drivers/hwmon/w83627hf.c 2007-10-12 17:42:11.000000000 -0600
> @@ -175,15 +175,10 @@ superio_exit(void)
>
> #define W83781D_REG_TEMP2_CONFIG 0x152
> #define W83781D_REG_TEMP3_CONFIG 0x252
> -#define W83781D_REG_TEMP(nr) ((nr = 3) ? (0x0250) : \
> - ((nr = 2) ? (0x0150) : \
> - (0x27)))
> -#define W83781D_REG_TEMP_HYST(nr) ((nr = 3) ? (0x253) : \
> - ((nr = 2) ? (0x153) : \
> - (0x3A)))
> -#define W83781D_REG_TEMP_OVER(nr) ((nr = 3) ? (0x255) : \
> - ((nr = 2) ? (0x155) : \
> - (0x39)))
> +/* these are zero-based, unlike config constants above */
> +static u16 w83781d_reg_temp[] = { 0x27, 0x150, 0x250 };
> +static u16 w83781d_reg_temp_hyst[] = { 0x3A, 0x153, 0x253 };
> +static u16 w83781d_reg_temp_over[] = { 0x39, 0x155, 0x255 };
Could be made const. Please also name these w83627hf_reg_* instead of
w83781d_reg_*. Ultimately we want to remove all references to w83781d
from this driver.
>
> #define W83781D_REG_BANK 0x4E
>
> @@ -360,12 +355,9 @@ struct w83627hf_data {
> u8 in_min[9]; /* Register value */
> u8 fan[3]; /* Register value */
> u8 fan_min[3]; /* Register value */
> - u8 temp;
> - u8 temp_max; /* Register value */
> - u8 temp_max_hyst; /* Register value */
> - u16 temp_add[2]; /* Register value */
> - u16 temp_max_add[2]; /* Register value */
> - u16 temp_max_hyst_add[2]; /* Register value */
> + u16 temp[3];
These are register values as well.
> + u16 temp_max[3]; /* Register value */
> + u16 temp_max_hyst[3]; /* Register value */
> u8 fan_div[3]; /* Register encoding, shifted right */
> u8 vid; /* Register encoding, combined */
> u32 alarms; /* Register encoding, combined */
> @@ -610,12 +602,10 @@ show_temp(struct device *dev, struct dev
> {
> int nr = to_sensor_dev_attr(devattr)->index;
> struct w83627hf_data *data = w83627hf_update_device(dev);
> - if (nr >= 2) { /* TEMP2 and TEMP3 */
> - return sprintf(buf, "%ld\n",
> - (long)LM75_TEMP_FROM_REG(data->temp_add[nr-2]));
> - } else { /* TEMP1 */
> - return sprintf(buf, "%ld\n", (long)TEMP_FROM_REG(data->temp));
> - }
> +
> + u16 tmp = data->temp[nr];
> + return sprintf(buf, "%ld\n", (nr) ? (long) LM75_TEMP_FROM_REG(tmp)
> + : (long) TEMP_FROM_REG(tmp));
> }
>
> static ssize_t
> @@ -624,13 +614,10 @@ show_temp_max(struct device *dev, struct
> {
> int nr = to_sensor_dev_attr(devattr)->index;
> struct w83627hf_data *data = w83627hf_update_device(dev);
> - if (nr >= 2) { /* TEMP2 and TEMP3 */
> - return sprintf(buf, "%ld\n",
> - (long)LM75_TEMP_FROM_REG(data->temp_max_add[nr-2]));
> - } else { /* TEMP1 */
> - return sprintf(buf, "%ld\n",
> - (long)TEMP_FROM_REG(data->temp_max));
> - }
> +
> + u16 tmp = data->temp_max[nr];
> + return sprintf(buf, "%ld\n", (nr) ? (long) LM75_TEMP_FROM_REG(tmp)
> + : (long) TEMP_FROM_REG(tmp));
> }
>
> static ssize_t
> @@ -639,13 +626,10 @@ show_temp_max_hyst(struct device *dev, s
> {
> int nr = to_sensor_dev_attr(devattr)->index;
> struct w83627hf_data *data = w83627hf_update_device(dev);
> - if (nr >= 2) { /* TEMP2 and TEMP3 */
> - return sprintf(buf, "%ld\n",
> - (long)LM75_TEMP_FROM_REG(data->temp_max_hyst_add[nr-2]));
> - } else { /* TEMP1 */
> - return sprintf(buf, "%ld\n",
> - (long)TEMP_FROM_REG(data->temp_max_hyst));
> - }
> +
> + u16 tmp = data->temp_max_hyst[nr];
> + return sprintf(buf, "%ld\n", (nr) ? (long) LM75_TEMP_FROM_REG(tmp)
> + : (long) TEMP_FROM_REG(tmp));
> }
>
> static ssize_t
> @@ -655,18 +639,16 @@ store_temp_max(struct device *dev, struc
> int nr = to_sensor_dev_attr(devattr)->index;
> struct w83627hf_data *data = dev_get_drvdata(dev);
> long val = simple_strtol(buf, NULL, 10);
> + u16 tmp;
>
> mutex_lock(&data->update_lock);
>
> - if (nr >= 2) { /* TEMP2 and TEMP3 */
> - data->temp_max_add[nr-2] = LM75_TEMP_TO_REG(val);
> - w83627hf_write_value(data, W83781D_REG_TEMP_OVER(nr),
> - data->temp_max_add[nr-2]);
> - } else { /* TEMP1 */
> - data->temp_max = TEMP_TO_REG(val);
> - w83627hf_write_value(data, W83781D_REG_TEMP_OVER(nr),
> - data->temp_max);
> - }
> + tmp = (nr) ? LM75_TEMP_TO_REG(val)
> + : TEMP_TO_REG(val);
This instruction could be moved outside of the lock-holding section.
> +
> + data->temp_max[nr] = tmp;
> + w83627hf_write_value(data, w83781d_reg_temp_over[nr], tmp);
> +
> mutex_unlock(&data->update_lock);
> return count;
> }
> @@ -678,29 +660,27 @@ store_temp_max_hyst(struct device *dev,
> int nr = to_sensor_dev_attr(devattr)->index;
> struct w83627hf_data *data = dev_get_drvdata(dev);
> long val = simple_strtol(buf, NULL, 10);
> + u16 tmp;
>
> mutex_lock(&data->update_lock);
>
> - if (nr >= 2) { /* TEMP2 and TEMP3 */
> - data->temp_max_hyst_add[nr-2] = LM75_TEMP_TO_REG(val);
> - w83627hf_write_value(data, W83781D_REG_TEMP_HYST(nr),
> - data->temp_max_hyst_add[nr-2]);
> - } else { /* TEMP1 */
> - data->temp_max_hyst = TEMP_TO_REG(val);
> - w83627hf_write_value(data, W83781D_REG_TEMP_HYST(nr),
> - data->temp_max_hyst);
> - }
> + tmp = (nr) ? LM75_TEMP_TO_REG(val)
> + : TEMP_TO_REG(val);
Same here.
> +
> + data->temp_max_hyst[nr] = tmp;
> + w83627hf_write_value(data, w83781d_reg_temp_hyst[nr], tmp);
> +
> mutex_unlock(&data->update_lock);
> return count;
> }
>
> #define sysfs_temp_decl(offset) \
> static SENSOR_DEVICE_ATTR(temp##offset##_input, S_IRUGO, \
> - show_temp, NULL, offset); \
> + show_temp, NULL, offset - 1); \
> static SENSOR_DEVICE_ATTR(temp##offset##_max, S_IRUGO|S_IWUSR, \
> - show_temp_max, store_temp_max, offset); \
> + show_temp_max, store_temp_max, offset - 1); \
> static SENSOR_DEVICE_ATTR(temp##offset##_max_hyst, S_IRUGO|S_IWUSR, \
> - show_temp_max_hyst, store_temp_max_hyst, offset);
> + show_temp_max_hyst, store_temp_max_hyst, offset - 1);
>
> sysfs_temp_decl(1);
> sysfs_temp_decl(2);
> @@ -1543,7 +1523,7 @@ static void __devinit w83627hf_init_devi
> static struct w83627hf_data *w83627hf_update_device(struct device *dev)
> {
> struct w83627hf_data *data = dev_get_drvdata(dev);
> - int i;
> + int i, num_temps = (data->type = w83697hf) ? 2 : 3;
>
> mutex_lock(&data->update_lock);
>
> @@ -1596,25 +1576,13 @@ static struct w83627hf_data *w83627hf_up
> break;
> }
> }
> -
> - data->temp = w83627hf_read_value(data, W83781D_REG_TEMP(1));
> - data->temp_max > - w83627hf_read_value(data, W83781D_REG_TEMP_OVER(1));
> - data->temp_max_hyst > - w83627hf_read_value(data, W83781D_REG_TEMP_HYST(1));
> - data->temp_add[0] > - w83627hf_read_value(data, W83781D_REG_TEMP(2));
> - data->temp_max_add[0] > - w83627hf_read_value(data, W83781D_REG_TEMP_OVER(2));
> - data->temp_max_hyst_add[0] > - w83627hf_read_value(data, W83781D_REG_TEMP_HYST(2));
> - if (data->type != w83697hf) {
> - data->temp_add[1] > - w83627hf_read_value(data, W83781D_REG_TEMP(3));
> - data->temp_max_add[1] > - w83627hf_read_value(data, W83781D_REG_TEMP_OVER(3));
> - data->temp_max_hyst_add[1] > - w83627hf_read_value(data, W83781D_REG_TEMP_HYST(3));
> + for (i=0; i<num_temps; i++) {
Coding style: add spaces around "=" and "<".
> + data->temp[i] = w83627hf_read_value(
> + data, w83781d_reg_temp[i]);
> + data->temp_max[i] = w83627hf_read_value(
> + data, w83781d_reg_temp_over[i]);
> + data->temp_max_hyst[i] = w83627hf_read_value(
> + data, w83781d_reg_temp_hyst[i]);
> }
>
> i = w83627hf_read_value(data, W83781D_REG_VID_FANDIV);
Other than these minor things, the patch looks OK and testing is OK as
well. Please send an updated version and I'll ack it.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [patch 1/1] w83627hf hoist nr-1 offset out of
2007-10-12 22:50 [lm-sensors] [patch 1/1] w83627hf hoist nr-1 offset out of Jim Cromie
2007-10-12 23:56 ` Jim Cromie
2007-10-14 8:36 ` Jean Delvare
@ 2007-10-14 23:10 ` Jim Cromie
2007-10-15 13:40 ` Jean Delvare
2007-10-16 10:46 ` Mark M. Hoffman
4 siblings, 0 replies; 6+ messages in thread
From: Jim Cromie @ 2007-10-14 23:10 UTC (permalink / raw)
To: lm-sensors
[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]
Jean Delvare wrote:
> Hi Jim,
>
> On Fri, 12 Oct 2007 17:56:41 -0600, Jim Cromie wrote:
>
>> this patch replaces last, fixing (long) cast placement,
>> and one previously missed (nr>=2) -> (nr)
>>
>> This hoists nr-1 offset out of (show|store)_temp_*(.*) callbacks, and into
>> SENSOR_DEVICE_ATTRs for sysfs tempN_X files. It also combines
>> temp[1] and temp_add[2] (array) fields in w83627hf_data into 3 elem arrays,
>> which simplifies special-case handling of nr, allowing simplification
>> of callback bodies and rerolling a flattened loop in
>> w83627hf_update_device(struct device *dev).
>>
>
> Very nice cleanup, thanks.
>
>
thank you.
respun per your feedback.
>
> Could be made const. Please also name these w83627hf_reg_* instead of
> w83781d_reg_*. Ultimately we want to remove all references to w83781d
> from this driver.
>
>
Done. I included W83627HF_REG_TEMP2,3_CONFIG since theyre
so close (lexically and semantically) that it'd more confusing to leave
them.
would you take a sed -e s/W83781D/W83627HF/
or a sed -e s/W83781D_REG/W83627HF_REG/ patch ?
or is it more selective than that ?
that said, the current names are a hint about chip & driver
heritage and commonality.
>
>> + tmp = (nr) ? LM75_TEMP_TO_REG(val)
>> + : TEMP_TO_REG(val);
>>
>
> This instruction could be moved outside of the lock-holding section.
>
>
OK - I turned these into decl+init statement,
and took out a little whitespace.
[-- Attachment #2: diff.hwmon-w83627hf-hoist-temp-offsets --]
[-- Type: text/plain, Size: 8190 bytes --]
Binary files hwmon-testing/arch/i386/boot/setup.elf and hwmon-hoist-temp/arch/i386/boot/setup.elf differ
diff -ruNp -X dontdiff -X exclude-diffs hwmon-testing/drivers/hwmon/w83627hf.c hwmon-hoist-temp/drivers/hwmon/w83627hf.c
--- hwmon-testing/drivers/hwmon/w83627hf.c 2007-10-14 13:00:24.000000000 -0600
+++ hwmon-hoist-temp/drivers/hwmon/w83627hf.c 2007-10-14 16:58:56.000000000 -0600
@@ -173,17 +173,12 @@ superio_exit(void)
#define W83781D_REG_FAN_MIN(nr) (0x3a + (nr))
#define W83781D_REG_FAN(nr) (0x27 + (nr))
-#define W83781D_REG_TEMP2_CONFIG 0x152
-#define W83781D_REG_TEMP3_CONFIG 0x252
-#define W83781D_REG_TEMP(nr) ((nr == 3) ? (0x0250) : \
- ((nr == 2) ? (0x0150) : \
- (0x27)))
-#define W83781D_REG_TEMP_HYST(nr) ((nr == 3) ? (0x253) : \
- ((nr == 2) ? (0x153) : \
- (0x3A)))
-#define W83781D_REG_TEMP_OVER(nr) ((nr == 3) ? (0x255) : \
- ((nr == 2) ? (0x155) : \
- (0x39)))
+#define W83627HF_REG_TEMP2_CONFIG 0x152
+#define W83627HF_REG_TEMP3_CONFIG 0x252
+/* these are zero-based, unlike config constants above */
+static const u16 w83627hf_reg_temp[] = { 0x27, 0x150, 0x250 };
+static const u16 w83627hf_reg_temp_hyst[] = { 0x3A, 0x153, 0x253 };
+static const u16 w83627hf_reg_temp_over[] = { 0x39, 0x155, 0x255 };
#define W83781D_REG_BANK 0x4E
@@ -360,12 +355,9 @@ struct w83627hf_data {
u8 in_min[9]; /* Register value */
u8 fan[3]; /* Register value */
u8 fan_min[3]; /* Register value */
- u8 temp;
- u8 temp_max; /* Register value */
- u8 temp_max_hyst; /* Register value */
- u16 temp_add[2]; /* Register value */
- u16 temp_max_add[2]; /* Register value */
- u16 temp_max_hyst_add[2]; /* Register value */
+ u16 temp[3]; /* Register value */
+ u16 temp_max[3]; /* Register value */
+ u16 temp_max_hyst[3]; /* Register value */
u8 fan_div[3]; /* Register encoding, shifted right */
u8 vid; /* Register encoding, combined */
u32 alarms; /* Register encoding, combined */
@@ -611,12 +603,10 @@ show_temp(struct device *dev, struct dev
{
int nr = to_sensor_dev_attr(devattr)->index;
struct w83627hf_data *data = w83627hf_update_device(dev);
- if (nr >= 2) { /* TEMP2 and TEMP3 */
- return sprintf(buf, "%ld\n",
- (long)LM75_TEMP_FROM_REG(data->temp_add[nr-2]));
- } else { /* TEMP1 */
- return sprintf(buf, "%ld\n", (long)TEMP_FROM_REG(data->temp));
- }
+
+ u16 tmp = data->temp[nr];
+ return sprintf(buf, "%ld\n", (nr) ? (long) LM75_TEMP_FROM_REG(tmp)
+ : (long) TEMP_FROM_REG(tmp));
}
static ssize_t
@@ -625,13 +615,10 @@ show_temp_max(struct device *dev, struct
{
int nr = to_sensor_dev_attr(devattr)->index;
struct w83627hf_data *data = w83627hf_update_device(dev);
- if (nr >= 2) { /* TEMP2 and TEMP3 */
- return sprintf(buf, "%ld\n",
- (long)LM75_TEMP_FROM_REG(data->temp_max_add[nr-2]));
- } else { /* TEMP1 */
- return sprintf(buf, "%ld\n",
- (long)TEMP_FROM_REG(data->temp_max));
- }
+
+ u16 tmp = data->temp_max[nr];
+ return sprintf(buf, "%ld\n", (nr) ? (long) LM75_TEMP_FROM_REG(tmp)
+ : (long) TEMP_FROM_REG(tmp));
}
static ssize_t
@@ -640,13 +627,10 @@ show_temp_max_hyst(struct device *dev, s
{
int nr = to_sensor_dev_attr(devattr)->index;
struct w83627hf_data *data = w83627hf_update_device(dev);
- if (nr >= 2) { /* TEMP2 and TEMP3 */
- return sprintf(buf, "%ld\n",
- (long)LM75_TEMP_FROM_REG(data->temp_max_hyst_add[nr-2]));
- } else { /* TEMP1 */
- return sprintf(buf, "%ld\n",
- (long)TEMP_FROM_REG(data->temp_max_hyst));
- }
+
+ u16 tmp = data->temp_max_hyst[nr];
+ return sprintf(buf, "%ld\n", (nr) ? (long) LM75_TEMP_FROM_REG(tmp)
+ : (long) TEMP_FROM_REG(tmp));
}
static ssize_t
@@ -656,18 +640,11 @@ store_temp_max(struct device *dev, struc
int nr = to_sensor_dev_attr(devattr)->index;
struct w83627hf_data *data = dev_get_drvdata(dev);
long val = simple_strtol(buf, NULL, 10);
+ u16 tmp = (nr) ? LM75_TEMP_TO_REG(val) : TEMP_TO_REG(val);
mutex_lock(&data->update_lock);
-
- if (nr >= 2) { /* TEMP2 and TEMP3 */
- data->temp_max_add[nr-2] = LM75_TEMP_TO_REG(val);
- w83627hf_write_value(data, W83781D_REG_TEMP_OVER(nr),
- data->temp_max_add[nr-2]);
- } else { /* TEMP1 */
- data->temp_max = TEMP_TO_REG(val);
- w83627hf_write_value(data, W83781D_REG_TEMP_OVER(nr),
- data->temp_max);
- }
+ data->temp_max[nr] = tmp;
+ w83627hf_write_value(data, w83627hf_reg_temp_over[nr], tmp);
mutex_unlock(&data->update_lock);
return count;
}
@@ -679,29 +656,22 @@ store_temp_max_hyst(struct device *dev,
int nr = to_sensor_dev_attr(devattr)->index;
struct w83627hf_data *data = dev_get_drvdata(dev);
long val = simple_strtol(buf, NULL, 10);
+ u16 tmp = (nr) ? LM75_TEMP_TO_REG(val) : TEMP_TO_REG(val);
mutex_lock(&data->update_lock);
-
- if (nr >= 2) { /* TEMP2 and TEMP3 */
- data->temp_max_hyst_add[nr-2] = LM75_TEMP_TO_REG(val);
- w83627hf_write_value(data, W83781D_REG_TEMP_HYST(nr),
- data->temp_max_hyst_add[nr-2]);
- } else { /* TEMP1 */
- data->temp_max_hyst = TEMP_TO_REG(val);
- w83627hf_write_value(data, W83781D_REG_TEMP_HYST(nr),
- data->temp_max_hyst);
- }
+ data->temp_max_hyst[nr] = tmp;
+ w83627hf_write_value(data, w83627hf_reg_temp_hyst[nr], tmp);
mutex_unlock(&data->update_lock);
return count;
}
#define sysfs_temp_decl(offset) \
static SENSOR_DEVICE_ATTR(temp##offset##_input, S_IRUGO, \
- show_temp, NULL, offset); \
+ show_temp, NULL, offset - 1); \
static SENSOR_DEVICE_ATTR(temp##offset##_max, S_IRUGO|S_IWUSR, \
- show_temp_max, store_temp_max, offset); \
+ show_temp_max, store_temp_max, offset - 1); \
static SENSOR_DEVICE_ATTR(temp##offset##_max_hyst, S_IRUGO|S_IWUSR, \
- show_temp_max_hyst, store_temp_max_hyst, offset);
+ show_temp_max_hyst, store_temp_max_hyst, offset - 1);
sysfs_temp_decl(1);
sysfs_temp_decl(2);
@@ -1514,23 +1484,23 @@ static void __devinit w83627hf_init_devi
if(init) {
/* Enable temp2 */
- tmp = w83627hf_read_value(data, W83781D_REG_TEMP2_CONFIG);
+ tmp = w83627hf_read_value(data, W83627HF_REG_TEMP2_CONFIG);
if (tmp & 0x01) {
dev_warn(&pdev->dev, "Enabling temp2, readings "
"might not make sense\n");
- w83627hf_write_value(data, W83781D_REG_TEMP2_CONFIG,
+ w83627hf_write_value(data, W83627HF_REG_TEMP2_CONFIG,
tmp & 0xfe);
}
/* Enable temp3 */
if (type != w83697hf) {
tmp = w83627hf_read_value(data,
- W83781D_REG_TEMP3_CONFIG);
+ W83627HF_REG_TEMP3_CONFIG);
if (tmp & 0x01) {
dev_warn(&pdev->dev, "Enabling temp3, "
"readings might not make sense\n");
w83627hf_write_value(data,
- W83781D_REG_TEMP3_CONFIG, tmp & 0xfe);
+ W83627HF_REG_TEMP3_CONFIG, tmp & 0xfe);
}
}
}
@@ -1563,7 +1533,7 @@ static void w83627hf_update_fan_div(stru
static struct w83627hf_data *w83627hf_update_device(struct device *dev)
{
struct w83627hf_data *data = dev_get_drvdata(dev);
- int i;
+ int i, num_temps = (data->type == w83697hf) ? 2 : 3;
mutex_lock(&data->update_lock);
@@ -1616,25 +1586,13 @@ static struct w83627hf_data *w83627hf_up
break;
}
}
-
- data->temp = w83627hf_read_value(data, W83781D_REG_TEMP(1));
- data->temp_max =
- w83627hf_read_value(data, W83781D_REG_TEMP_OVER(1));
- data->temp_max_hyst =
- w83627hf_read_value(data, W83781D_REG_TEMP_HYST(1));
- data->temp_add[0] =
- w83627hf_read_value(data, W83781D_REG_TEMP(2));
- data->temp_max_add[0] =
- w83627hf_read_value(data, W83781D_REG_TEMP_OVER(2));
- data->temp_max_hyst_add[0] =
- w83627hf_read_value(data, W83781D_REG_TEMP_HYST(2));
- if (data->type != w83697hf) {
- data->temp_add[1] =
- w83627hf_read_value(data, W83781D_REG_TEMP(3));
- data->temp_max_add[1] =
- w83627hf_read_value(data, W83781D_REG_TEMP_OVER(3));
- data->temp_max_hyst_add[1] =
- w83627hf_read_value(data, W83781D_REG_TEMP_HYST(3));
+ for (i = 0; i < num_temps; i++) {
+ data->temp[i] = w83627hf_read_value(
+ data, w83627hf_reg_temp[i]);
+ data->temp_max[i] = w83627hf_read_value(
+ data, w83627hf_reg_temp_over[i]);
+ data->temp_max_hyst[i] = w83627hf_read_value(
+ data, w83627hf_reg_temp_hyst[i]);
}
w83627hf_update_fan_div(data);
[-- Attachment #3: Type: text/plain, Size: 153 bytes --]
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [patch 1/1] w83627hf hoist nr-1 offset out of
2007-10-12 22:50 [lm-sensors] [patch 1/1] w83627hf hoist nr-1 offset out of Jim Cromie
` (2 preceding siblings ...)
2007-10-14 23:10 ` Jim Cromie
@ 2007-10-15 13:40 ` Jean Delvare
2007-10-16 10:46 ` Mark M. Hoffman
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2007-10-15 13:40 UTC (permalink / raw)
To: lm-sensors
Hi Jim,
On Sun, 14 Oct 2007 17:10:52 -0600, Jim Cromie wrote:
> Jean Delvare wrote:
> > Hi Jim,
> >
> > On Fri, 12 Oct 2007 17:56:41 -0600, Jim Cromie wrote:
> >
> >> this patch replaces last, fixing (long) cast placement,
> >> and one previously missed (nr>=2) -> (nr)
> >>
> >> This hoists nr-1 offset out of (show|store)_temp_*(.*) callbacks, and into
> >> SENSOR_DEVICE_ATTRs for sysfs tempN_X files. It also combines
> >> temp[1] and temp_add[2] (array) fields in w83627hf_data into 3 elem arrays,
> >> which simplifies special-case handling of nr, allowing simplification
> >> of callback bodies and rerolling a flattened loop in
> >> w83627hf_update_device(struct device *dev).
> >>
> >
> > Very nice cleanup, thanks.
> >
> >
> thank you.
>
> respun per your feedback.
> >
> > Could be made const. Please also name these w83627hf_reg_* instead of
> > w83781d_reg_*. Ultimately we want to remove all references to w83781d
> > from this driver.
>
>
> Done. I included W83627HF_REG_TEMP2,3_CONFIG since theyre
> so close (lexically and semantically) that it'd more confusing to leave
> them.
Fine with me.
> would you take a sed -e s/W83781D/W83627HF/
> or a sed -e s/W83781D_REG/W83627HF_REG/ patch ?
> or is it more selective than that ?
I would be fine with the former, to start with. But there are also
lowercase conversions to make, and some references that need to be
updated. For example, having references to the W83782D in a driver that
doesn't support that chip doesn't make much sense.
> that said, the current names are a hint about chip & driver
> heritage and commonality.
The drivers have diverged so much by now that we don't really care.
New patch looks good to me:
Acked-by: Jean Delvare <khali@linux-fr.org>
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [patch 1/1] w83627hf hoist nr-1 offset out of
2007-10-12 22:50 [lm-sensors] [patch 1/1] w83627hf hoist nr-1 offset out of Jim Cromie
` (3 preceding siblings ...)
2007-10-15 13:40 ` Jean Delvare
@ 2007-10-16 10:46 ` Mark M. Hoffman
4 siblings, 0 replies; 6+ messages in thread
From: Mark M. Hoffman @ 2007-10-16 10:46 UTC (permalink / raw)
To: lm-sensors
Hi Jim:
* Jim Cromie <jim.cromie@gmail.com> [2007-10-14 17:10:52 -0600]:
> Binary files hwmon-testing/arch/i386/boot/setup.elf and hwmon-hoist-temp/arch/i386/boot/setup.elf differ
> diff -ruNp -X dontdiff -X exclude-diffs hwmon-testing/drivers/hwmon/w83627hf.c hwmon-hoist-temp/drivers/hwmon/w83627hf.c
> --- hwmon-testing/drivers/hwmon/w83627hf.c 2007-10-14 13:00:24.000000000 -0600
> +++ hwmon-hoist-temp/drivers/hwmon/w83627hf.c 2007-10-14 16:58:56.000000000 -0600
When you respin a patch following review, next time please include the patch
description and Signed-off-by again. Applied to hwmon-2.6.git/testing, thanks.
--
Mark M. Hoffman
mhoffman@lightlink.com
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread