Skip to contentSkip to conversation

Misc cleanups to the w83792d driver

PATCH In review Jean Delvare 2 participants 4 messages Nov 28, 2005 – Nov 30, 2005 Hardware monitoring

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Misc cleanups to the w83792d driver

Hi Yuan, hi Rudolf,

Here's a w83792d patch, comments welcome. If there are no objections I'll enqueue it for Linux 2.6.16.

Thanks.

* * * * *

Cleanup the w83792d driver a bit:
* Discard unused lock and irrelevant comments inherited from the w83781d driver.
* Inline and simplify w83792d_{read,write}_value functions.
* Drop useless address test during detection.

---
 drivers/hwmon/w83792d.c |   29 ++++++++++-------------------
 1 files changed, 10 insertions(+), 19 deletions(-)
drivers/hwmon/w83792d.c +10 -19
@@ -268,7 +268,6 @@
 struct w83792d_data {
 	struct i2c_client client;
 	struct class_device *class_dev;
-	struct semaphore lock;
 	enum chips type;
 
 	struct semaphore update_lock;
@@ -304,8 +303,8 @@
 static int w83792d_detect(struct i2c_adapter *adapter, int address, int kind);
 static int w83792d_detach_client(struct i2c_client *client);
 
-static int w83792d_read_value(struct i2c_client *client, u8 register);
-static int w83792d_write_value(struct i2c_client *client, u8 register,
+static inline int w83792d_read_value(struct i2c_client *client, u8 register);
+static inline int w83792d_write_value(struct i2c_client *client, u8 register,
 				u8 value);
 static struct w83792d_data *w83792d_update_device(struct device *dev);
 
@@ -1192,7 +1191,6 @@
 	new_client = &data->client;
 	i2c_set_clientdata(new_client, data);
 	new_client->addr = address;
-	init_MUTEX(&data->lock);
 	new_client->adapter = adapter;
 	new_client->driver = &w83792d_driver;
 	new_client->flags = 0;
@@ -1243,7 +1241,7 @@
 			goto ERROR1;
 		}
 		val1 = w83792d_read_value(new_client, W83792D_REG_WCHIPID);
-		if (val1 = 0x7a && address >= 0x2c) {
+		if (val1 = 0x7a) {
 			kind = w83792d;
 		} else {
 			if (kind = 0)
@@ -1406,26 +1404,19 @@
 	return 0;
 }
 
-/* The SMBus locks itself, usually, but nothing may access the Winbond between
-   bank switches. ISA access must always be locked explicitly!
-   We ignore the W83792D BUSY flag at this moment - it could lead to deadlocks,
-   would slow down the W83792D access and should not be necessary.
-   There are some ugly typecasts here, but the good news is - they should
-   nowhere else be necessary! */
-static int
+/* The SMBus locks itself. The Winbond W83792D chip has a bank register,
+   but the driver only accesses registers in bank 0, so we don't have
+   to switch banks and lock access between switches. */
+static inline int
 w83792d_read_value(struct i2c_client *client, u8 reg)
 {
-	int res=0;
-	res = i2c_smbus_read_byte_data(client, reg);
-
-	return res;
+	return i2c_smbus_read_byte_data(client, reg);
 }
 
-static int
+static inline int
 w83792d_write_value(struct i2c_client *client, u8 reg, u8 value)
 {
-	i2c_smbus_write_byte_data(client, reg,  value);
-	return 0;
+	return i2c_smbus_write_byte_data(client, reg, value);
 }
 
 static void
Signature · 1 line
Jean Delvare
#1 lore raw

Hi Jean,
It Seems OK :)
How about in_count_from_reg function, would you please simplify this function if you also think it worth to do.
I think get low bits do not need so much code.
Thanks.

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.本信件內所含華邦電子的財產性機密性資訊, 僅授權原發信人指定之收信人取閱\之用. 假使您並非被指定之收信人或因任何原因在未經授權的情形之下收到本信件, 請您告知原發信人並立即將信件從電腦與網路伺服器中予以消除. 對於您的合作, 我們先此致謝. 特此提醒, 任何未經授權擅自使用華邦電子的機密資訊的行為是被嚴格禁止的. 信件與華邦電子營業無關之內容,不得視為華邦電子之立場或意見.

#2 lore raw
In reply to Jean Delvare

Hi Yuan,

Quoted 4 lines · Yuan Mu

It Seems OK :)
How about in_count_from_reg function, would you please simplify this function if you also think it worth to do.
I think get low bits do not need so much code.

What about the attached patch? Please test it, if it works OK for you I'll get it applied.

--
Jean Delvare
-------------- next part --------------
Content-Disposition: inline; filename=hwmon-w83792d-simplify-in-low-bits.patch

Simplify the way the w83792d driver handles the extra resolution bits of voltage input channels.

 drivers/hwmon/w83792d.c |   48 ++++++++++--------------------------------------
 1 files changed, 10 insertions(+), 38 deletions(-)
drivers/hwmon/w83792d.c +10 -38
@@ -281,7 +281,7 @@
 	u8 in[9];		/* Register value */
 	u8 in_max[9];		/* Register value */
 	u8 in_min[9];		/* Register value */
-	u8 low_bits[2];		/* Additional resolution to voltage in0-6 */
+	u16 low_bits;		/* Additional resolution to voltage in6-0 */
 	u8 fan[7];		/* Register value */
 	u8 fan_min[7];		/* Register value */
 	u8 temp1[3];		/* current, over, thyst */
@@ -324,38 +324,10 @@
 	.detach_client = w83792d_detach_client,
 };
 
-static long in_count_from_reg(int nr, struct w83792d_data *data)
+static inline long in_count_from_reg(int nr, struct w83792d_data *data)
 {
-	u16 vol_count = data->in[nr];
-	u16 low_bits = 0;
-	vol_count = (vol_count << 2);
-	switch (nr)
-	{
-	case 0:  /* vin0 */
-		low_bits = (data->low_bits[0]) & 0x03;
-		break;
-	case 1:  /* vin1 */
-		low_bits = ((data->low_bits[0]) & 0x0c) >> 2;
-		break;
-	case 2:  /* vin2 */
-		low_bits = ((data->low_bits[0]) & 0x30) >> 4;
-		break;
-	case 3:  /* vin3 */
-		low_bits = ((data->low_bits[0]) & 0xc0) >> 6;
-		break;
-	case 4:  /* vin4 */
-		low_bits = (data->low_bits[1]) & 0x03;
-		break;
-	case 5:  /* vin5 */
-		low_bits = ((data->low_bits[1]) & 0x0c) >> 2;
-		break;
-	case 6:  /* vin6 */
-		low_bits = ((data->low_bits[1]) & 0x30) >> 4;
-	default:
-		break;
-	}
-	vol_count = vol_count | low_bits;
-	return vol_count;
+	/* in7 and in8 do not have low bits, but the formula still works */
+	return ((data->in[nr] << 2) | ((data->low_bits >> (2 * nr)) & 0x03));
 }
 
 /* following are the sysfs callback functions */
@@ -1484,10 +1456,10 @@
 			data->in_min[i] = w83792d_read_value(client,
 						W83792D_REG_IN_MIN[i]);
 		}
-		data->low_bits[0] = w83792d_read_value(client,
-						W83792D_REG_LOW_BITS1);
-		data->low_bits[1] = w83792d_read_value(client,
-						W83792D_REG_LOW_BITS2);
+		data->low_bits = w83792d_read_value(client,
+						W83792D_REG_LOW_BITS1) +
+				 (w83792d_read_value(client,
+						W83792D_REG_LOW_BITS2) << 8);
 		for (i = 0; i < 7; i++) {
 			/* Update the Fan measured value and limits */
 			data->fan[i] = w83792d_read_value(client,
@@ -1599,8 +1571,8 @@
 		dev_dbg(dev, "vin[%d] max is: 0x%x\n", i, data->in_max[i]);
 		dev_dbg(dev, "vin[%d] min is: 0x%x\n", i, data->in_min[i]);
 	}
-	dev_dbg(dev, "Low Bit1 is: 0x%x\n", data->low_bits[0]);
-	dev_dbg(dev, "Low Bit2 is: 0x%x\n", data->low_bits[1]);
+	dev_dbg(dev, "Low Bit1 is: 0x%x\n", data->low_bits & 0xff);
+	dev_dbg(dev, "Low Bit2 is: 0x%x\n", data->low_bits >> 8);
 	dev_dbg(dev, "7 set of Fan Counts and Duty Cycles: ===>\n");
 	for (i=0; i<7; i++) {
 		dev_dbg(dev, "fan[%d] is: 0x%x\n", i, data->fan[i]);
#3 lore raw
In reply to Jean Delvare

Hi Jean,

It works OK for me ;)

Best Regards
Yuan Mu

Quoted 2 lines

-----Original Message-----
From: [email protected]

[mailto:[email protected]] On Behalf Of Jean Delvare

Quoted 13 lines

Sent: Tuesday, November 29, 2005 9:49 PM
To: PI14 YMU
Cc: Rudolf Marek; LM Sensors
Subject: [lm-sensors] RE: [PATCH] Misc cleanups to the w83792d driver

Hi Yuan,

Quoted 4 lines · Yuan Mu

It Seems OK :)
How about in_count_from_reg function, would you please simplify this function if you also think it worth to do.
I think get low bits do not need so much code.

Quoted 5 lines

What about the attached patch? Please test it, if it works OK for you I'll get it applied.

--
Jean Delvare

==============================================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.本信件內所含華邦電子的財產性機密性資訊, 僅授權原發信人指定之收信人取閱\之用. 假使您並非被指定之收信人或因任何原因在未經授權的情形之下收到本信件, 請您告知原發信人並立即將信件從電腦與網路伺服器中予以消除. 對於您的合作, 我們先此致謝. 特此提醒, 任何未經授權擅自使用華邦電子的機密資訊的行為是被嚴格禁止的. 信件與華邦電子營業無關之內容,不得視為華邦電子之立場或意見.