Hi Jim,
On Fri, 12 Oct 2007 20:03:26 -0600, Jim Cromie wrote:
> patch changes 2 macros to incorporate the +1, and drops the +1 from
> all the callers.
> This also allows a 'reroll' of an expanded loop, and adjusting indexes
> and loop limits
> on another.
>
> Signed-off-by: Jim Cromie <jim.cromie at gmail.com>
> ---
> drivers/hwmon/w83627hf.c | 22 +++++++++++-----------
> 2 files changed, 11 insertions(+), 11 deletions(-)
>
> this gives a small shrink : 22 bytes on i686
> 12850 2652 36 15538 3cb2 hwmon-hf-1/drivers/hwmon/w83627hf.ko
> 12818 2652 36 15506 3c92 hwmon-hf-2/drivers/hwmon/w83627hf.ko
Review:
> --- hwmon-hf-1/drivers/hwmon/w83627hf.c 2007-10-12 17:42:11.000000000 -0600
> +++ hwmon-hf-2/drivers/hwmon/w83627hf.c 2007-10-12 18:03:23.000000000 -0600
> @@ -170,8 +170,8 @@ superio_exit(void)
> #define W83781D_REG_IN(nr) ((nr < 7) ? (0x20 + (nr)) : \
> (0x550 + (nr) - 7))
>
> -#define W83781D_REG_FAN_MIN(nr) (0x3a + (nr))
> -#define W83781D_REG_FAN(nr) (0x27 + (nr))
> +#define W83781D_REG_FAN_MIN(nr) (0x3b + (nr))
> +#define W83781D_REG_FAN(nr) (0x28 + (nr))
Here again, please rename these W83627HF_* while you're here. Maybe
also tab-align the definitions for readability. I would also appreciate
a comment giving the valid values of nr, just to clear up any possible
confusion.
>
> #define W83781D_REG_TEMP2_CONFIG 0x152
> #define W83781D_REG_TEMP3_CONFIG 0x252
> @@ -581,7 +581,7 @@ store_fan_min(struct device *dev, struct
>
> mutex_lock(&data->update_lock);
> data->fan_min[nr] = FAN_TO_REG(val, DIV_FROM_REG(data->fan_div[nr]));
> - w83627hf_write_value(data, W83781D_REG_FAN_MIN(nr+1),
> + w83627hf_write_value(data, W83781D_REG_FAN_MIN(nr),
> data->fan_min[nr]);
>
> mutex_unlock(&data->update_lock);
> @@ -823,7 +823,7 @@ store_fan_div(struct device *dev, struct
>
> /* Restore fan_min */
> data->fan_min[nr] = FAN_TO_REG(min, DIV_FROM_REG(data->fan_div[nr]));
> - w83627hf_write_value(data, W83781D_REG_FAN_MIN(nr+1), data->fan_min[nr]);
> + w83627hf_write_value(data, W83781D_REG_FAN_MIN(nr), data->fan_min[nr]);
>
> mutex_unlock(&data->update_lock);
> return count;
> @@ -1149,7 +1149,7 @@ static int __devinit w83627hf_probe(stru
> struct w83627hf_sio_data *sio_data = dev->platform_data;
> struct w83627hf_data *data;
> struct resource *res;
> - int err;
> + int err, i;
>
> static const char *names[] = {
> "w83627hf",
> @@ -1183,9 +1183,9 @@ static int __devinit w83627hf_probe(stru
> w83627hf_init_device(pdev);
>
> /* A few vars need to be filled upon startup */
> - data->fan_min[0] = w83627hf_read_value(data, W83781D_REG_FAN_MIN(1));
> - data->fan_min[1] = w83627hf_read_value(data, W83781D_REG_FAN_MIN(2));
> - data->fan_min[2] = w83627hf_read_value(data, W83781D_REG_FAN_MIN(3));
> + for (i = 0; i <= 2; i++)
> + data->fan_min[i] =
> + w83627hf_read_value(data, W83781D_REG_FAN_MIN(i));
At this point your patch conflicts with one I sent a few days ago:
hwmon: (w83627hf) Fix setting fan min right after driver load
http://lm-sensors.org/kernel?p=kernel/mhoffman/hwmon-2.6.git;a=commitdiff;h=c09c5184a26158da32801e89d5849d774605f0dd
Please make sure you have this patch applied on your local tree before
regenerating and resending your patch.
>
> /* Register common device attributes */
> if ((err = sysfs_create_group(&dev->kobj, &w83627hf_group)))
> @@ -1544,10 +1544,10 @@ static struct w83627hf_data *w83627hf_up
> w83627hf_read_value(data,
> W83781D_REG_IN_MAX(i));
> }
> - for (i = 1; i <= 3; i++) {
> - data->fan[i - 1] =
> + for (i = 0; i <= 2; i++) {
> + data->fan[i] =
> w83627hf_read_value(data, W83781D_REG_FAN(i));
> - data->fan_min[i - 1] =
> + data->fan_min[i] =
> w83627hf_read_value(data,
> W83781D_REG_FAN_MIN(i));
> }
Rest looks OK and testing is OK as well.
Thanks,
--
Jean Delvare
