Skip to contentSkip to conversation

i2c-i801: Fix minor style issues

PATCH Superseded Ivo Manca 2 participants 4 messages Jul 23, 2008 – Aug 13, 2008 3 messages not in the archive I2C subsystem host drivers

lore
Share & tools
lore

Ivo Manca

author
#0 lore raw

PATCH 3/3 i2c-i801: Fix minor style issues

This patch fixes minor style issues warned about by checkPatch

---

[PATCH 00/03] i2c-i801: Add basic interrupt support

_new renamed from __new +6 -7
@@ -381,9 +381,8 @@ static int i801_block_transaction_byte_b
 		do {
 			msleep(1);
 			temp = inb_p(SMBHSTSTS);
-		}
-		while ((!(temp & SMBHSTSTS_BYTE_DONE))
-		       && (timeout++ < MAX_TIMEOUT));
+		} while ((!(temp & SMBHSTSTS_BYTE_DONE))
+			 && (timeout++ < MAX_TIMEOUT));
 
 		/* If the SMBus is still busy, we give up */
 		if (timeout >= MAX_TIMEOUT) {
@@ -502,7 +501,7 @@ static int i801_block_transaction(union 
 }
 
 /* Return -1 on error. */
-static s32 i801_access(struct i2c_adapter * adap, u16 addr,
+static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		       unsigned short flags, char read_write, u8 command,
 		       int size, union i2c_smbus_data * data)
 {
@@ -574,7 +573,7 @@ static s32 i801_access(struct i2c_adapte
 	else
 		outb_p(inb_p(SMBAUXCTL) & (~SMBAUXCTL_CRC), SMBAUXCTL);
 
-	if(block)
+	if (block)
 		ret = i801_block_transaction(data, read_write, size, hwpec);
 	else
 		ret = i801_transaction(xact | ENABLE_INT9);
@@ -586,9 +585,9 @@ static s32 i801_access(struct i2c_adapte
 		outb_p(inb_p(SMBAUXCTL) & ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B),
 		       SMBAUXCTL);
 
-	if(block)
+	if (block)
 		return ret;
-	if(ret)
+	if (ret)
 		return -1;
 	if ((read_write == I2C_SMBUS_WRITE) || (xact == I801_QUICK))
 		return 0;

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

#2 lore raw
In reply to a message not in the archive
Quoted 41 lines · Ivo Manca

This patch fixes minor style issues warned about by checkPatch

---

[PATCH 00/03] i2c-i801: Add basic interrupt support

_new renamed from __new +4 -5
@@ -381,9 +381,8 @@ static int i801_block_transaction_byte_b
 		do {
 			msleep(1);
 			temp = inb_p(SMBHSTSTS);
-		}
-		while ((!(temp & SMBHSTSTS_BYTE_DONE))
-		       && (timeout++ < MAX_TIMEOUT));
+		} while ((!(temp & SMBHSTSTS_BYTE_DONE))
+			 && (timeout++ < MAX_TIMEOUT));
 
 		/* If the SMBus is still busy, we give up */
 		if (timeout >= MAX_TIMEOUT) {
@@ -502,7 +501,7 @@ static int i801_block_transaction(union 
 }
 
 /* Return -1 on error. */
-static s32 i801_access(struct i2c_adapter * adap, u16 addr,
+static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		       unsigned short flags, char read_write, u8 command,
 		       int size, union i2c_smbus_data * data)
 {
@@ -574,7 +573,7 @@ static s32 i801_access(struct i2c_adapte
 	else
 		outb_p(inb_p(SMBAUXCTL) & (~SMBAUXCTL_CRC), SMBAUXCTL);
 
-	if(block)
+	if (block)
 		ret = i801_block_transaction(data, read_write, size, hwpec);
 	else
 		ret = i801_transaction(xact | ENABLE_INT9);
@@ -586,9 +585,9 @@ static s32 i801_access(struct i2c_adapte
 		outb_p(inb_p(SMBAUXCTL) & ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B),

_new static s32 i801_access(struct i2c_adapte · line 593

Quoted 10 lines
_new renamed from __new +2 -2
 		       SMBAUXCTL);
 
-	if(block)
+	if (block)
 		return ret;
-	if(ret)
+	if (ret)
 		return -1;
 	if ((read_write == I2C_SMBUS_WRITE) || (xact == I801_QUICK))
 		return 0;

Looks OK, thanks for doing that.

I'm now waiting for an update of your patches based on 2.6.27-rc2 for a final review.

Oh, one last note before I forget: as the interrupt-based logic is new and might not be as robust as the old poll-based one, it might make sense to give the user a way to disable the interrupt-based logic and fall back to polling in case the new code doesn't work correctly. Without that possibility, I won't feel too confident to push your patches to Linus. Remember that the ICH chips are very popular and we just can't afford breaking these systems.

This could be implemented as a build time option enabling the new interrupt-based code, tagged EXPERIMENTAL and disabled by default, or a module parameter, or both.

Thanks,

Signature · 6 lines
Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
author
#4 lore raw
In reply to a message not in the archive
Quoted 3 lines

Looks OK, thanks for doing that.

Seemed to be the easiest and best way to shut checkpatch up ;)

Quoted 3 lines

I'm now waiting for an update of your patches based on 2.6.27-rc2 for a final review.

I hope to have them finnished and sent tomorrow.

Quoted 2 lines

Oh, one last note before I forget: as the interrupt-based logic is new and might not be as robust as the old poll-based one,

Likely

Quoted 7 lines

it might make
sense to give the user a way to disable the interrupt-based logic and fall back to polling in case the new code doesn't work correctly. Without that possibility, I won't feel too confident to push your patches to Linus. Remember that the ICH chips are very popular and we just can't afford breaking these systems.

True, i'd rather have that option as well.

Quoted 4 lines

This could be implemented as a build time option enabling the new interrupt-based code, tagged EXPERIMENTAL and disabled by default, or a module parameter, or both.

What option do you think will generate more use and thus more testing? A build time option (which is disabled by default thus won't be used by the generic user) or a module parameter which people might not know the existance of, but is easy to enable? Not sure what the best option is :).

Quoted 2 lines

Thanks,

Ivo

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

#6 lore raw
In reply to a message not in the archive
Quoted 11 lines · Ivo Manca
Quoted 2 lines

Oh, one last note before I forget: as the interrupt-based logic is new and might not be as robust as the old poll-based one,

Likely

Quoted 7 lines

it might make
sense to give the user a way to disable the interrupt-based logic and fall back to polling in case the new code doesn't work correctly. Without that possibility, I won't feel too confident to push your patches to Linus. Remember that the ICH chips are very popular and we just can't afford breaking these systems.

True, i'd rather have that option as well.

Quoted 8 lines
Quoted 4 lines

This could be implemented as a build time option enabling the new interrupt-based code, tagged EXPERIMENTAL and disabled by default, or a module parameter, or both.

What option do you think will generate more use and thus more testing? A build time option (which is disabled by default thus won't be used by the generic user) or a module parameter which people might not know the existance of, but is easy to enable? Not sure what the best option is :).

I think I would have a module parameter those default value is determined by a configuration option. The default mode would be polling first, then after some time we can change the default to interrupt mode. And after some more time we remove the option. And maybe after some more time (several years) we even remove the module parameter.

This seems like a reasonable upgrade path, which will let the users help us with the testing on a voluntary basis while still not putting the other users at risk.

Signature · 6 lines
Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c