[lm-sensors] [PATCH] hwmon : Fix W83627THF VID reading

[lm-sensors] [PATCH] hwmon : Fix W83627THF VID reading

[Ymu]

Hello,
This patch fixes the VID reading; no cpu0_vid and vrm files created if
the chip is w83627thf and GPIO5 not enabled.
Please check.

Signed-off-by: Yuan Mu <ymu@winbond.com.tw>

--- linux-2.6.14-rc5.orig/drivers/hwmon/w83627hf.c	2005-11-09
11:27:44.000000000 +0800
+++ linux-2.6.14-rc5/drivers/hwmon/w83627hf.c	2005-11-15
14:05:14.000000000 +0800
@@ -1116,11 +1116,11 @@ static int w83627hf_detect(struct i2c_ad
 	if (kind != w83697hf)
 		device_create_file_temp(new_client, 3);
 
-	if (kind != w83697hf)
+	if (kind != w83697hf &&
+		!(kind = w83627thf && data->vid = 0xff)) {
 		device_create_file_vid(new_client);
-
-	if (kind != w83697hf)
 		device_create_file_vrm(new_client);
+	}
 
 	device_create_file_fan_div(new_client, 1);
 	device_create_file_fan_div(new_client, 2);
@@ -1318,19 +1318,18 @@ static void w83627hf_init_client(struct 
 		int hi = w83627hf_read_value(client,
W83781D_REG_CHIPID);
 		data->vid = (lo & 0x0f) | ((hi & 0x01) << 4);
 	} else if (w83627thf = data->type) {
-		data->vid = w83627thf_read_gpio5(client) & 0x3f;
+		data->vid = w83627thf_read_gpio5(client);
 	}
 
 	/* Read VRM & OVT Config only once */
 	if (w83627thf = data->type || w83637hf = data->type) {
 		data->vrm_ovt = 
 			w83627hf_read_value(client,
W83627THF_REG_VRM_OVT_CFG);
-		data->vrm = (data->vrm_ovt & 0x01) ? 90 : 82;
-	} else {
-		/* Convert VID to voltage based on default VRM */
-		data->vrm = vid_which_vrm();
 	}
 
+	/* Convert VID to voltage based on default VRM */
+	data->vrm = vid_which_vrm();
+
 	tmp = w83627hf_read_value(client, W83781D_REG_SCFG1);
 	for (i = 1; i <= 3; i++) {
 		if (!(tmp & BIT_SCFG1[i - 1])) {


Best Regards
Yuan Mu


==============================================The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such  a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Winbond is strictly prohibited; and any information in this email irrelevant to the official business of Winbond shall be deemed as neither given nor endorsed by Winbond.
==============================================If your computer is unable to decode Chinese font, please ignore the following message.It essentially repeats the statement in English given above.本信件內所含華邦電子的財產性機密性資訊, 僅授權原發信人指定之收信人取閱\之用. 假使您並非被指定之收信人或因任何原因在未經授權的情形之下收到本信件, 請您告知原發信人並立即將信件從電腦與網路伺服器中予以消除. 對於您的合作, 我們先此致謝. 特此提醒, 任何未經授權擅自使用華邦電子的機密資訊的行為是被嚴格禁止的. 信件與華邦電子營業無關之內容,不得視為華邦電子之立場或意見.

[lm-sensors] Re: [PATCH] hwmon : Fix W83627THF VID reading

[Jean Delvare]

Hi Yuan,

> This patch fixes the VID reading; no cpu0_vid and vrm files created if
> the chip is w83627thf and GPIO5 not enabled.
> Please check.

Looks good to me. I had to fix the patch manually this time again though.

I have done three minor edits to your patch, I hope you won't mind:

1* I think the following test:
+	if (kind != w83697hf &&
+		!(kind = w83627thf && data->vid = 0xff)) {
Can be simplified to:
+	if (kind != w83697hf && data->vid != 0xff)) {
Because data->vid can only have value 0xff for type w83627thf anyway.

2* In w83627thf_read_gpio5, I did:
-	sel = superio_inb(W83627THF_GPIO5_IOSR);
+	sel = superio_inb(W83627THF_GPIO5_IOSR) & 0x3f;
The datasheet suggests that the two unused bits of register CRF4
(W83627THF_GPIO5_DR) are always zero, but better safe than sorry.

3* I dropped "default" in that comment:
        /* Convert VID to voltage based on default VRM */
It's not more a default value, it's deduced from the CPU model.

I'll stack your patch and will get it into -mm soon so that it gets some
testing. It should then go into mainline for 2.6.16. Thanks for your
contribution!

While checking the W83627THF datasheet, I noticed that there is an
additional GPIO configuration register, CRF5, named "inversion
register". Wouldn't it make sense to check the value of this register
and make sure all pins supposedly used as VID inputs are not inverted?
Mark, can you please confirm that your system which does use GPIO5 for
VID would pass that test? Proposed patch attached, comments and testers
welcome.

Thanks,
--
Jean Delvare
-------------- next part --------------
Improve the W83627THF GPIO5 testing, so that we don't think these pins
are used as VID inputs when they cannot be. This is done by checking
that none of these pins are in inverted mode.
---

 drivers/hwmon/w83627hf.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

--- linux-2.6.15-rc1.orig/drivers/hwmon/w83627hf.c	2005-11-15 11:03:52.000000000 +0100
+++ linux-2.6.15-rc1/drivers/hwmon/w83627hf.c	2005-11-15 11:19:49.000000000 +0100
@@ -99,6 +99,7 @@
 #define W83627THF_GPIO5_EN	0x30 /* w83627thf only */
 #define W83627THF_GPIO5_IOSR	0xf3 /* w83627thf only */
 #define W83627THF_GPIO5_DR	0xf4 /* w83627thf only */
+#define W83627THF_GPIO5_INV	0xf5 /* w83627thf only */
 
 static inline void
 superio_outb(int reg, int val)
@@ -1218,7 +1219,7 @@
 
 static int w83627thf_read_gpio5(struct i2c_client *client)
 {
-	int res = 0xff, sel;
+	int res = 0xff, sel, inv;
 
 	superio_enter();
 	superio_select(W83627HF_LD_GPIO5);
@@ -1232,7 +1233,8 @@
 	/* Make sure the pins are configured for input
 	   There must be at least five (VRM 9), and possibly 6 (VRM 10) */
 	sel = superio_inb(W83627THF_GPIO5_IOSR) & 0x3f;
-	if ((sel & 0x1f) != 0x1f) {
+	inv = superio_inb(W83627THF_GPIO5_INV) & 0x3f;
+	if ((sel & 0x1f) != 0x1f || (inv & sel) != 0x00) {
 		dev_dbg(&client->dev, "GPIO5 not configured for VID "
 			"function\n");
 		goto exit;

[lm-sensors] Re: [PATCH] hwmon : Fix W83627THF VID reading

[Mark M. Hoffman]

Hi Jean:

* Jean Delvare <khali@linux-fr.org> [2005-11-15 12:16:52 +0200]:
> While checking the W83627THF datasheet, I noticed that there is an
> additional GPIO configuration register, CRF5, named "inversion
> register". Wouldn't it make sense to check the value of this register
> and make sure all pins supposedly used as VID inputs are not inverted?
> Mark, can you please confirm that your system which does use GPIO5 for
> VID would pass that test? Proposed patch attached, comments and testers
> welcome.

Your patch does work on my system, but... no, I don't think it means anything
if the inputs are inverted or not.  The BIOS might set the inputs to inverted
if the VID signals are inverted by hardware... which I would not consider to
be unusual or unexpected.

Of course, it might just as well leave them *not* inverted *despite* that
they are inverted in hardware... which suggests a new module option if anyone
ever discovers such a board.

BTW: I tested by applying your current quilt stack (up to and including the
improve-gpio5-test patch) to 2.6.15-rc2.  

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

[lm-sensors] Re: [PATCH] hwmon : Fix W83627THF VID reading

[Jean Delvare]

Hi Mark,

On 2005-11-20, Mark M. Hoffman wrote:
> * Jean Delvare [2005-11-15 12:16:52 +0200]:
> > While checking the W83627THF datasheet, I noticed that there is an
> > additional GPIO configuration register, CRF5, named "inversion
> > register". Wouldn't it make sense to check the value of this register
> > and make sure all pins supposedly used as VID inputs are not inverted?
> > Mark, can you please confirm that your system which does use GPIO5 for
> > VID would pass that test? Proposed patch attached, comments and testers
> > welcome.
>
> Your patch does work on my system, but... no, I don't think it means
> anything if the inputs are inverted or not.  The BIOS might set the inputs
> to inverted if the VID signals are inverted by hardware... which I would
> not consider to be unusual or unexpected.

I do consider it to be unexpected. The W83627HF cannot invert these pins
as far as I know, so if the W83627THF is supposed to be used as a
replacement for the W83627HF, the GPIO pins shouldn't be inverted.
Additionally, the VID signals are supposed to come directly from the
CPU, and if they do they cannot be inverted.

> Of course, it might just as well leave them *not* inverted *despite* that
> they are inverted in hardware... which suggests a new module option if
> anyone ever discovers such a board.

That would be a different problem (getting the proper value, as opposed
to properly guessing if the pins are used as VID inputs), although the
same reasons I gave above make me doubt this could actually happen.

> BTW: I tested by applying your current quilt stack (up to and including
> the improve-gpio5-test patch) to 2.6.15-rc2.

Great, thanks. I'll probably push upwards a part of it this week so that
it gets a broader testing in -mm.

--
Jean Delvare

[lm-sensors] Re: [PATCH] hwmon : Fix W83627THF VID reading

[Jean Delvare]

Hi again Mark,

> > Your patch does work on my system, but... no, I don't think it means
> > anything if the inputs are inverted or not.  The BIOS might set the inputs
> > to inverted if the VID signals are inverted by hardware... which I would
> > not consider to be unusual or unexpected.
> 
> I do consider it to be unexpected. The W83627HF cannot invert these pins
> as far as I know, so if the W83627THF is supposed to be used as a
> replacement for the W83627HF, the GPIO pins shouldn't be inverted.
> Additionally, the VID signals are supposed to come directly from the
> CPU, and if they do they cannot be inverted.

On second though, you're right. We just can't assume anything. I've
backed out the patch I had proposed.

> Great, thanks. I'll probably push upwards a part of it this week so that
> it gets a broader testing in -mm.

I'll do later this afternoon, BTW.

Thanks,
-- 
Jean Delvare