Skip to contentSkip to conversation

hwmon: w83792d misc cleanups

PATCH New Jean Delvare 2 participants 5 messages Dec 18, 2005 – Dec 28, 2005 Hardware monitoring

lore
Share & tools
lore

Jean Delvare

author
#0 lore raw

PATCH 1/11 hwmon: w83792d misc cleanups

Content-Disposition: inline; filename=hwmon-w83792d-misc-cleanups.patch

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.
* Drop useless bitmasking.

Signed-off-by Jean Delvare <khali at linux-fr.org>
Acked-by Yuan Mu <Ymu at winbond.com.tw>
---
 drivers/hwmon/w83792d.c |   31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)
drivers/hwmon/w83792d.c +11 -20
@@ -269,7 +269,6 @@
 struct w83792d_data {
 	struct i2c_client client;
 	struct class_device *class_dev;
-	struct semaphore lock;
 	enum chips type;
 
 	struct semaphore update_lock;

@@ -305,8 +304,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)

@@ -1416,26 +1414,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

@@ -1506,7 +1497,7 @@
 			pwm_array_tmp[i] = w83792d_read_value(client,
 						W83792D_REG_PWM[i]);
 			data->pwm[i] = pwm_array_tmp[i] & 0x0f;
-			data->pwm_mode[i] = (pwm_array_tmp[i] >> 7) & 0x01;
+			data->pwm_mode[i] = pwm_array_tmp[i] >> 7;
 		}
 
 		reg_tmp = w83792d_read_value(client, W83792D_REG_FAN_CFG);
Signature · 1 line
Jean Delvare
#1 lore raw

PATCH 1/11 hwmon: w83792d misc cleanups

drivers/hwmon/w83792d.c line 309

Quoted 5 lines · Jean Delvare
drivers/hwmon/w83792d.c +2 -2
-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);

Will cause compile errors, so I've dropped this portion of the patch (see the patch I sent back to you for details.)

thanks,

greg k-h

author
#2 lore raw

PATCH 1/11 hwmon: w83792d misc cleanups

In reply to Jean Delvare

Hi Greg,

Quoted 9 lines · Jean Delvare

drivers/hwmon/w83792d.c line 309

Quoted 5 lines
drivers/hwmon/w83792d.c +2 -2
-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);

Will cause compile errors, so I've dropped this portion of the patch (see the patch I sent back to you for details.)

Can you explain please? We have four other hardware monitoring drivers with similar code, and no problem was ever reported. This change to the w83792d driver was compiling just fine for me with both gcc 2.95.3 and gcc 3.3.6. What exactly is supposed to cause problems, and with which compiler?

Thanks,

Signature · 1 line
Jean Delvare
#3 lore raw

PATCH 1/11 hwmon: w83792d misc cleanups

In reply to Jean Delvare
Quoted 12 lines · Jean Delvare

Hi Greg,

Quoted 9 lines · Jean Delvare

drivers/hwmon/w83792d.c line 309

Quoted 5 lines
drivers/hwmon/w83792d.c +2 -2
-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);

Will cause compile errors, so I've dropped this portion of the patch (see the patch I sent back to you for details.)

Quoted 5 lines

Can you explain please? We have four other hardware monitoring drivers with similar code, and no problem was ever reported. This change to the w83792d driver was compiling just fine for me with both gcc 2.95.3 and gcc 3.3.6. What exactly is supposed to cause problems, and with which compiler?

I'm using gcc 3.4.4 here, and this change caused build errors. You can't declare a function "static inline" and not actually declare the full function before calling it, like this driver did (it called w83792d_write_value() before that function was fully written.) I think older versions of gcc just ignore this and don't inline it.

The other drivers that do this, properly define the functions fully before calling them, that's why it works.

Hope this helps,

greg k-h

author
#4 lore raw

PATCH 1/11 hwmon: w83792d misc cleanups

In reply to Jean Delvare

Hi Greg,

Quoted 6 lines
Quoted 5 lines

Can you explain please? We have four other hardware monitoring drivers with similar code, and no problem was ever reported. This change to the w83792d driver was compiling just fine for me with both gcc 2.95.3 and gcc 3.3.6. What exactly is supposed to cause problems, and with which compiler?

Quoted 8 lines

I'm using gcc 3.4.4 here, and this change caused build errors. You can't declare a function "static inline" and not actually declare the full function before calling it, like this driver did (it called w83792d_write_value() before that function was fully written.) I think older versions of gcc just ignore this and don't inline it.

The other drivers that do this, properly define the functions fully before calling them, that's why it works.

OK, thanks for the clarification. It all makes perfect sense now. Here comes a fixup patch. Can you please fold it into your version of hwmon-w83792d-misc-cleanups.patch? Thanks.

Signed-off-by Jean Delvare <khali at linux-fr.org>
Acked-by Yuan Mu <Ymu at winbond.com.tw>
---
 drivers/hwmon/w83792d.c |   31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)
drivers/hwmon/w83792d.c +14 -17
@@ -303,10 +303,6 @@
 static int w83792d_attach_adapter(struct i2c_adapter *adapter);
 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,
-				u8 value);
 static struct w83792d_data *w83792d_update_device(struct device *dev);
 
 #ifdef DEBUG

@@ -329,6 +325,20 @@
 	return ((data->in[nr] << 2) | ((data->low_bits >> (2 * nr)) & 0x03));
 }
 
+/* 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)
+{
+	return i2c_smbus_read_byte_data(client, reg);
+}
+
+static inline int
+w83792d_write_value(struct i2c_client *client, u8 reg, u8 value)
+{
+	return i2c_smbus_write_byte_data(client, reg, value);
+}
+
 /* following are the sysfs callback functions */
 static ssize_t show_in(struct device *dev, struct device_attribute *attr,
 			char *buf)

@@ -1386,19 +1396,6 @@
 	return 0;
 }
 
-/* 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 int w83792d_read_value(struct i2c_client *client, u8 reg)
-{
-	return i2c_smbus_read_byte_data(client, reg);
-}
-
-static int w83792d_write_value(struct i2c_client *client, u8 reg, u8 value)
-{
-	return i2c_smbus_write_byte_data(client, reg, value);
-}
-
 static void
 w83792d_init_client(struct i2c_client *client)
 {
Signature · 1 line
Jean Delvare