Re: [lm-sensors] w83627ehf: Wrong values reported after resuming from suspend/hibernation

[Harald Judt <h.judt@gmx.at>]

Hi Jean,

Am 24.10.2012 10:39, schrieb Jean Delvare:
> On Tue, 23 Oct 2012 21:02:40 +0200, Harald Judt wrote:
>> Am 23.10.2012 18:32, schrieb Jean Delvare:
>>> I have attached a shell script which will dump all registers from the
>>> chip. I would like you to run it before and after suspend, and send me
>>> the results. That way I can ensure that I save and restore all relevant
>>> registers.
>>>
>>> # rmmod w83627ehf
>>> # ./isadump_all_banks.sh 0x295 0x296 before-suspend
>>> (suspend, resume)
>>> # ./isadump_all_banks.sh 0x295 0x296 after-resume
>>>
>>> This will generate 8 dump files, please send them back to me.
>>
>> Here are the 8 dumps generated by the script.
>
> Thank you. I have come up with a first implementation of suspend/resume
> support for the w83627ehf driver. Is is available as a patch below, but
> also as a standalone driver at:
>    http://khali.linux-fr.org/devel/misc/w83627ehf/
> Instructions at:
>   http://khali.linux-fr.org/devel/misc/INSTALL
>
> From: Jean Delvare <khali@linux-fr.org>
> Subject: hwmon: (w83627ehf) Add support for suspend
>
> On suspend some register values are lost, most notably the Value RAM
> areas but also other limits and settings. Restore them on resume.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> ---
>   drivers/hwmon/w83627ehf.c |   93 ++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 92 insertions(+), 1 deletion(-)
>
> --- linux-3.7-rc2.orig/drivers/hwmon/w83627ehf.c	2012-10-23 11:29:19.000000000 +0200
> +++ linux-3.7-rc2/drivers/hwmon/w83627ehf.c	2012-10-24 10:12:36.752171223 +0200
> @@ -1,7 +1,7 @@
>   /*
>    *  w83627ehf - Driver for the hardware monitoring functionality of
>    *		the Winbond W83627EHF Super-I/O chip
> - *  Copyright (C) 2005-2011  Jean Delvare <khali@linux-fr.org>
> + *  Copyright (C) 2005-2012  Jean Delvare <khali@linux-fr.org>
>    *  Copyright (C) 2006  Yuan Mu (Winbond),
>    *			Rudolf Marek <r.marek@assembler.cz>
>    *			David Hubbard <david.c.hubbard@gmail.com>
> @@ -502,6 +502,13 @@ struct w83627ehf_data {
>   	u16 have_temp_offset;
>   	u8 in6_skip:1;
>   	u8 temp3_val_only:1;
> +
> +#ifdef CONFIG_PM
> +	/* Remember extra register values over suspend/resume */
> +	u8 vbat;
> +	u8 fandiv1;
> +	u8 fandiv2;
> +#endif
>   };
>
>   struct w83627ehf_sio_data {
> @@ -2607,10 +2614,94 @@ static int __devexit w83627ehf_remove(st
>   	return 0;
>   }
>
> +#ifdef CONFIG_PM
> +static int w83627ehf_suspend(struct device *dev)
> +{
> +	struct w83627ehf_data *data = w83627ehf_update_device(dev);
> +	struct w83627ehf_sio_data *sio_data = dev->platform_data;
> +
> +	mutex_lock(&data->update_lock);
> +	data->vbat = w83627ehf_read_value(data, W83627EHF_REG_VBAT);
> +	if (sio_data->kind = nct6775) {
> +		data->fandiv1 = w83627ehf_read_value(data, NCT6775_REG_FANDIV1);
> +		data->fandiv2 = w83627ehf_read_value(data, NCT6775_REG_FANDIV2);
> +	}
> +	mutex_unlock(&data->update_lock);
> +
> +	return 0;
> +}
> +
> +static int w83627ehf_resume(struct device *dev)
> +{
> +	struct w83627ehf_data *data = dev_get_drvdata(dev);
> +	struct w83627ehf_sio_data *sio_data = dev->platform_data;
> +	int i;
> +
> +	/* Restore limits */
> +	mutex_lock(&data->update_lock);
> +	for (i = 0; i < data->in_num; i++) {
> +		if ((i = 6) && data->in6_skip)
> +			continue;
> +
> +		w83627ehf_write_value(data, W83627EHF_REG_IN_MIN(i),
> +				      data->in_min[i]);
> +		w83627ehf_write_value(data, W83627EHF_REG_IN_MAX(i),
> +				      data->in_max[i]);
> +	}
> +
> +	for (i = 0; i < 5; i++) {
> +		if (!(data->has_fan_min & (1 << i)))
> +			continue;
> +
> +		w83627ehf_write_value(data, data->REG_FAN_MIN[i],
> +				      data->fan_min[i]);
> +	}
> +
> +	for (i = 0; i < NUM_REG_TEMP; i++) {
> +		if (!(data->have_temp & (1 << i)))
> +			continue;
> +
> +		if (data->reg_temp_over[i])
> +			w83627ehf_write_temp(data, data->reg_temp_over[i],
> +					     data->temp_max[i]);
> +		if (data->reg_temp_hyst[i])
> +			w83627ehf_write_temp(data, data->reg_temp_hyst[i],
> +					     data->temp_max_hyst[i]);
> +		if (data->have_temp_offset & (1 << i))
> +			w83627ehf_write_value(data,
> +					      W83627EHF_REG_TEMP_OFFSET[i],
> +					      data->temp_offset[i]);
> +	}
> +
> +	/* Restore other settings */
> +	w83627ehf_write_value(data, W83627EHF_REG_VBAT, data->vbat);
> +	if (sio_data->kind = nct6775) {
> +		w83627ehf_write_value(data, NCT6775_REG_FANDIV1, data->fandiv1);
> +		w83627ehf_write_value(data, NCT6775_REG_FANDIV2, data->fandiv2);
> +	}
> +
> +	/* Force re-reading all values */
> +	data->valid = 0;
> +	mutex_unlock(&data->update_lock);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops w83627ehf_dev_pm_ops = {
> +	.suspend = w83627ehf_suspend,
> +	.resume = w83627ehf_resume,
> +};
> +
> +#define W83627EHF_DEV_PM_OPS	(&w83627ehf_dev_pm_ops)
> +#else
> +#define W83627EHF_DEV_PM_OPS	NULL
> +#endif /* CONFIG_PM */
> +
>   static struct platform_driver w83627ehf_driver = {
>   	.driver = {
>   		.owner	= THIS_MODULE,
>   		.name	= DRVNAME,
> +		.pm	= W83627EHF_DEV_PM_OPS,
>   	},
>   	.probe		= w83627ehf_probe,
>   	.remove		= __devexit_p(w83627ehf_remove),
>
>

Thank you for your patch. I've applied it on 3.6.2, and it seems to work 
fine. The values are saved and restored correctly, and they also keep 
changing after resume. Further it gave me a little insight in how 
suspend/resume code works.

Best regards,
Harald

-- 
`Experience is the best teacher.'

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors