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