[lm-sensors] [PATCH] hwmon: (atxp1) Fix device detection logic

[lm-sensors] [PATCH] hwmon: (atxp1) Fix device detection logic

[Jean Delvare]

The atxp1 device detection code has a major logic flaw, fix it. Not
sure how we managed to miss this when the driver was merged...

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Sebastian Witt <se.witt@gmx.net>
---
Sebastian, can you please review and test this patch? I've tested with
i2c-stub but an additional test with a real device can't hurt.

 drivers/hwmon/atxp1.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- linux-2.6.26-rc8.orig/drivers/hwmon/atxp1.c	2008-06-29 11:52:24.000000000 +0200
+++ linux-2.6.26-rc8/drivers/hwmon/atxp1.c	2008-06-29 12:25:49.000000000 +0200
@@ -31,7 +31,7 @@
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("System voltages control via Attansic ATXP1");
-MODULE_VERSION("0.6.2");
+MODULE_VERSION("0.6.3");
 MODULE_AUTHOR("Sebastian Witt <se.witt@gmx.net>");
 
 #define ATXP1_VID	0x00
@@ -298,7 +298,8 @@ static int atxp1_detect(struct i2c_adapt
 	     (i2c_smbus_read_byte_data(new_client, 0x3f) = 0) &&
 	     (i2c_smbus_read_byte_data(new_client, 0xfe) = 0) &&
 	     (i2c_smbus_read_byte_data(new_client, 0xff) = 0) )) {
-
+		goto exit_free;
+	} else {
 		/* No vendor ID, now checking if registers 0x10,0x11 (non-existent)
 		 * showing the same as register 0x00 */
 		temp = i2c_smbus_read_byte_data(new_client, 0x00);


-- 
Jean Delvare

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

[lm-sensors] [PATCH] hwmon: (atxp1) Fix device detection logic

[Jean Delvare]

Subject: hwmon: (atxp1) Fix device detection logic

The atxp1 device detection code has a major logic flaw, fix it. Not
sure how we managed to miss this when the driver was merged...

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Sebastian Witt <se.witt@gmx.net>
---
Patch refreshed to apply on top of 2.6.27-rc3.

I didn't hear of Sebastian in 6 weeks. Can anyone please review this?

 drivers/hwmon/atxp1.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

--- linux-2.6.27-rc3.orig/drivers/hwmon/atxp1.c	2008-08-05 18:17:05.000000000 +0200
+++ linux-2.6.27-rc3/drivers/hwmon/atxp1.c	2008-08-13 19:37:43.000000000 +0200
@@ -31,7 +31,7 @@
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("System voltages control via Attansic ATXP1");
-MODULE_VERSION("0.6.2");
+MODULE_VERSION("0.6.3");
 MODULE_AUTHOR("Sebastian Witt <se.witt@gmx.net>");
 
 #define ATXP1_VID	0x00
@@ -289,16 +289,16 @@ static int atxp1_detect(struct i2c_clien
 	if (!((i2c_smbus_read_byte_data(new_client, 0x3e) = 0) &&
 	     (i2c_smbus_read_byte_data(new_client, 0x3f) = 0) &&
 	     (i2c_smbus_read_byte_data(new_client, 0xfe) = 0) &&
-	     (i2c_smbus_read_byte_data(new_client, 0xff) = 0) )) {
+	     (i2c_smbus_read_byte_data(new_client, 0xff) = 0)))
+		return -ENODEV;
 
-		/* No vendor ID, now checking if registers 0x10,0x11 (non-existent)
-		 * showing the same as register 0x00 */
-		temp = i2c_smbus_read_byte_data(new_client, 0x00);
-
-		if (!((i2c_smbus_read_byte_data(new_client, 0x10) = temp) &&
-			 (i2c_smbus_read_byte_data(new_client, 0x11) = temp) ))
-			return -ENODEV;
-	}
+	/* No vendor ID, now checking if registers 0x10,0x11 (non-existent)
+	 * showing the same as register 0x00 */
+	temp = i2c_smbus_read_byte_data(new_client, 0x00);
+
+	if (!((i2c_smbus_read_byte_data(new_client, 0x10) = temp) &&
+	      (i2c_smbus_read_byte_data(new_client, 0x11) = temp)))
+		return -ENODEV;
 
 	/* Get VRM */
 	temp = vid_which_vrm();


-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] hwmon: (atxp1) Fix device detection logic

[Jean Delvare]

Hi Sebastian,

Please keep the lm-sensors list Cc'd.

On Thu, 14 Aug 2008 00:09:32 +0200, Sebastian Witt wrote:
> Hello Jean,
> 
> sorry, missed your mail. The test sequence was:
> 
> 1. If vendor ID registers are all zero, assume ATXP1.
> 2. If not, do second test. If it succeeds it is a ATXP1.
> 
> I'm not sure anymore why it was this way, maybe I thought there would be a ATXP1 
> with vendor ID so it works for both...

That's indeed what the current code does, but that's not correct. We
want to check that _both_ conditions are met. Even with that, the
detection is rather weak... Without that it's very, very weak.

> However aborting when the first test fails and doing always the second test 
> should be fine, but I can't test it because I gave away the board some time ago.

If you look at the comments in the original code, that's what they say:
we have "No vendor ID" inside the block which is executed when there
_are_ IDs. So it's clear to me that the code doesn't do what we
intended to do in the first place, and was "working" only by accident.

Even if you can't test my patch, can you please review it and add your
Acked-by, so that I can push it upstream? Thanks.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] hwmon: (atxp1) Fix device detection logic

[Sebastian Witt]

Jean Delvare wrote:
> Subject: hwmon: (atxp1) Fix device detection logic
> 
> The atxp1 device detection code has a major logic flaw, fix it. Not
> sure how we managed to miss this when the driver was merged...
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
Acked-by: Sebastian Witt <se.witt@gmx.net>
> Cc: Sebastian Witt <se.witt@gmx.net>
> ---
> Patch refreshed to apply on top of 2.6.27-rc3.
> 
> I didn't hear of Sebastian in 6 weeks. Can anyone please review this?
> 
>  drivers/hwmon/atxp1.c |   20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> --- linux-2.6.27-rc3.orig/drivers/hwmon/atxp1.c	2008-08-05 18:17:05.000000000 +0200
> +++ linux-2.6.27-rc3/drivers/hwmon/atxp1.c	2008-08-13 19:37:43.000000000 +0200
> @@ -31,7 +31,7 @@
>  
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("System voltages control via Attansic ATXP1");
> -MODULE_VERSION("0.6.2");
> +MODULE_VERSION("0.6.3");
>  MODULE_AUTHOR("Sebastian Witt <se.witt@gmx.net>");
>  
>  #define ATXP1_VID	0x00
> @@ -289,16 +289,16 @@ static int atxp1_detect(struct i2c_clien
>  	if (!((i2c_smbus_read_byte_data(new_client, 0x3e) = 0) &&
>  	     (i2c_smbus_read_byte_data(new_client, 0x3f) = 0) &&
>  	     (i2c_smbus_read_byte_data(new_client, 0xfe) = 0) &&
> -	     (i2c_smbus_read_byte_data(new_client, 0xff) = 0) )) {
> +	     (i2c_smbus_read_byte_data(new_client, 0xff) = 0)))
> +		return -ENODEV;
>  
> -		/* No vendor ID, now checking if registers 0x10,0x11 (non-existent)
> -		 * showing the same as register 0x00 */
> -		temp = i2c_smbus_read_byte_data(new_client, 0x00);
> -
> -		if (!((i2c_smbus_read_byte_data(new_client, 0x10) = temp) &&
> -			 (i2c_smbus_read_byte_data(new_client, 0x11) = temp) ))
> -			return -ENODEV;
> -	}
> +	/* No vendor ID, now checking if registers 0x10,0x11 (non-existent)
> +	 * showing the same as register 0x00 */
> +	temp = i2c_smbus_read_byte_data(new_client, 0x00);
> +
> +	if (!((i2c_smbus_read_byte_data(new_client, 0x10) = temp) &&
> +	      (i2c_smbus_read_byte_data(new_client, 0x11) = temp)))
> +		return -ENODEV;
>  
>  	/* Get VRM */
>  	temp = vid_which_vrm();
> 
> 


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