Skip to contentSkip to conversation

Refactor swap_bytes in i2c chip drivers

PATCH New Jean Delvare 2 participants 6 messages May 19, 2005

lore
Share & tools
lore

Jean Delvare

author
#0 lore raw

PATCH 1/1 Refactor swap_bytes in i2c chip drivers

Hi Greg,

This simple patch refactors the swap_bytes function that various chip drivers use. I moved it to lm75.h because all drivers using lm75-like temperature sensors will most necessarily need to swap bytes.

Apply if you like it,
thanks.

Note #1: There's one more driver, gl518sm, that still has swap_bytes. I did not factor this one because it's not using the other functions provided by lm75.c (nor has it to).

Note #2: Looks like each time we manipulate 16-bit registers we end up swapping bytes. It looks like we chose the wrong order as the default. I fear it's a bit late to change that though.

drivers/i2c/chips/asb100.c renamed from drivers/i2c/chips.orig/asb100.c +0 -5
diff -ruN linux-2.6.5-rc2/drivers/i2c/chips.orig/asb100.c linux-2.6.5-rc2/drivers/i2c/chips/asb100.c
@@ -862,11 +862,6 @@
 	return 0;
 }
 
-static u16 swap_bytes(u16 val)
-{
-	return (val >> 8) | (val << 8);
-}
-
 /* The SMBus locks itself, usually, but nothing may access the chip between
    bank switches. */
 static int asb100_read_value(struct i2c_client *client, u16 reg)
drivers/i2c/chips/ds1621.c renamed from drivers/i2c/chips.orig/ds1621.c +0 -5
diff -ruN linux-2.6.5-rc2/drivers/i2c/chips.orig/ds1621.c linux-2.6.5-rc2/drivers/i2c/chips/ds1621.c
@@ -97,11 +97,6 @@
 
 static int ds1621_id = 0;
 
-static u16 swap_bytes(u16 val)
-{
-	return (val >> 8) | (val << 8);
-}
-
 /* All registers are word-sized, except for the configuration register.
    DS1621 uses a high-byte first convention, which is exactly opposite to
    the usual practice. */
drivers/i2c/chips/lm75.c renamed from drivers/i2c/chips.orig/lm75.c +0 -5
diff -ruN linux-2.6.5-rc2/drivers/i2c/chips.orig/lm75.c linux-2.6.5-rc2/drivers/i2c/chips/lm75.c
@@ -206,11 +206,6 @@
 	return 0;
 }
 
-static u16 swap_bytes(u16 val)
-{
-	return (val >> 8) | (val << 8);
-}
-
 /* All registers are word-sized, except for the configuration register.
    LM75 uses a high-byte first convention, which is exactly opposite to
    the usual practice. */
drivers/i2c/chips/lm75.h renamed from drivers/i2c/chips.orig/lm75.h +4 -0
diff -ruN linux-2.6.5-rc2/drivers/i2c/chips.orig/lm75.h linux-2.6.5-rc2/drivers/i2c/chips/lm75.h
@@ -47,3 +47,7 @@
 	return ((s16)reg / 128) * 500;
 }
 
+static inline u16 swap_bytes(u16 val)
+{
+	return (val >> 8) | (val << 8);
+}
drivers/i2c/chips/w83781d.c renamed from drivers/i2c/chips.orig/w83781d.c +0 -5
diff -ruN linux-2.6.5-rc2/drivers/i2c/chips.orig/w83781d.c linux-2.6.5-rc2/drivers/i2c/chips/w83781d.c
@@ -275,11 +275,6 @@
 static struct w83781d_data *w83781d_update_device(struct device *dev);
 static void w83781d_init_client(struct i2c_client *client);
 
-static inline u16 swap_bytes(u16 val)
-{
-	return (val >> 8) | (val << 8);
-}
-
 static struct i2c_driver w83781d_driver = {
 	.owner = THIS_MODULE,
 	.name = "w83781d",
Signature · 2 lines
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/
#1 lore raw

PATCH 1/1 Refactor swap_bytes in i2c chip drivers

Quoted 7 lines · Jean Delvare

Hi Greg,

This simple patch refactors the swap_bytes function that various chip drivers use. I moved it to lm75.h because all drivers using lm75-like temperature sensors will most necessarily need to swap bytes.

Apply if you like it,

Ick, no, we should be using the proper kernel call for this, swab16(), right? Care to redo this patch to just fix the drivers and get rid of our duplicating of this function.

thanks,

greg k-h

author
#2 lore raw

PATCH 1/1 Refactor swap_bytes in i2c chip drivers

In reply to Jean Delvare
Quoted 3 lines

Ick, no, we should be using the proper kernel call for this, swab16(), right? Care to redo this patch to just fix the drivers and get rid of our duplicating of this function.

Oh, I didn't know such a function existed, sorry.

Here's a new patch, hope you like it. Tested to work on my as99127f, btw (w83781d driver).

Documentation update follows (well, tomorrow it does).

Thanks.

drivers/i2c/chips/asb100.c renamed from drivers/i2c/chips.orig/asb100.c +5 -10
diff -ruN linux-2.6.5-rc2/drivers/i2c/chips.orig/asb100.c linux-2.6.5-rc2/drivers/i2c/chips/asb100.c
@@ -862,11 +862,6 @@
 	return 0;
 }
 
-static u16 swap_bytes(u16 val)
-{
-	return (val >> 8) | (val << 8);
-}
-
 /* The SMBus locks itself, usually, but nothing may access the chip between
    bank switches. */
 static int asb100_read_value(struct i2c_client *client, u16 reg)

@@ -891,17 +886,17 @@
 		/* convert from ISA to LM75 I2C addresses */
 		switch (reg & 0xff) {
 		case 0x50: /* TEMP */
-			res = swap_bytes(i2c_smbus_read_word_data (cl, 0));
+			res = swab16(i2c_smbus_read_word_data (cl, 0));
 			break;
 		case 0x52: /* CONFIG */
 			res = i2c_smbus_read_byte_data(cl, 1);
 			break;
 		case 0x53: /* HYST */
-			res = swap_bytes(i2c_smbus_read_word_data (cl, 2));
+			res = swab16(i2c_smbus_read_word_data (cl, 2));
 			break;
 		case 0x55: /* MAX */
 		default:
-			res = swap_bytes(i2c_smbus_read_word_data (cl, 3));
+			res = swab16(i2c_smbus_read_word_data (cl, 3));
 			break;
 		}
 	}

@@ -939,10 +934,10 @@
 			i2c_smbus_write_byte_data(cl, 1, value & 0xff);
 			break;
 		case 0x53: /* HYST */
-			i2c_smbus_write_word_data(cl, 2, swap_bytes(value));
+			i2c_smbus_write_word_data(cl, 2, swab16(value));
 			break;
 		case 0x55: /* MAX */
-			i2c_smbus_write_word_data(cl, 3, swap_bytes(value));
+			i2c_smbus_write_word_data(cl, 3, swab16(value));
 			break;
 		}
 	}
drivers/i2c/chips/ds1621.c renamed from drivers/i2c/chips.orig/ds1621.c +2 -8
diff -ruN linux-2.6.5-rc2/drivers/i2c/chips.orig/ds1621.c linux-2.6.5-rc2/drivers/i2c/chips/ds1621.c
@@ -97,11 +97,6 @@
 
 static int ds1621_id = 0;
 
-static u16 swap_bytes(u16 val)
-{
-	return (val >> 8) | (val << 8);
-}
-
 /* All registers are word-sized, except for the configuration register.
    DS1621 uses a high-byte first convention, which is exactly opposite to
    the usual practice. */

@@ -110,7 +105,7 @@
 	if (reg = DS1621_REG_CONF)
 		return i2c_smbus_read_byte_data(client, reg);
 	else
-		return swap_bytes(i2c_smbus_read_word_data(client, reg));
+		return swab16(i2c_smbus_read_word_data(client, reg));
 }
 
 /* All registers are word-sized, except for the configuration register.

@@ -121,8 +116,7 @@
 	if (reg = DS1621_REG_CONF)
 		return i2c_smbus_write_byte_data(client, reg, value);
 	else
-		return i2c_smbus_write_word_data(client, reg,
-						 swap_bytes(value));
+		return i2c_smbus_write_word_data(client, reg, swab16(value));
 }
 
 static void ds1621_init_client(struct i2c_client *client)
drivers/i2c/chips/gl518sm.c renamed from drivers/i2c/chips.orig/gl518sm.c +2 -8
diff -ruN linux-2.6.5-rc2/drivers/i2c/chips.orig/gl518sm.c linux-2.6.5-rc2/drivers/i2c/chips/gl518sm.c
@@ -484,18 +484,13 @@
 	return 0;
 }
 
-static inline u16 swap_bytes(u16 val)
-{
-	return (val >> 8) | (val << 8);
-}
-
 /* Registers 0x07 to 0x0c are word-sized, others are byte-sized 
    GL518 uses a high-byte first convention, which is exactly opposite to
    the usual practice. */
 static int gl518_read_value(struct i2c_client *client, u8 reg)
 {
 	if ((reg >= 0x07) && (reg <= 0x0c))
-		return swap_bytes(i2c_smbus_read_word_data(client, reg));
+		return swab16(i2c_smbus_read_word_data(client, reg));
 	else
 		return i2c_smbus_read_byte_data(client, reg);
 }

@@ -506,8 +501,7 @@
 static int gl518_write_value(struct i2c_client *client, u8 reg, u16 value)
 {
 	if ((reg >= 0x07) && (reg <= 0x0c))
-		return i2c_smbus_write_word_data(client, reg,
-						 swap_bytes(value));
+		return i2c_smbus_write_word_data(client, reg, swab16(value));
 	else
 		return i2c_smbus_write_byte_data(client, reg, value);
 }
drivers/i2c/chips/lm75.c renamed from drivers/i2c/chips.orig/lm75.c +2 -8
diff -ruN linux-2.6.5-rc2/drivers/i2c/chips.orig/lm75.c linux-2.6.5-rc2/drivers/i2c/chips/lm75.c
@@ -206,11 +206,6 @@
 	return 0;
 }
 
-static u16 swap_bytes(u16 val)
-{
-	return (val >> 8) | (val << 8);
-}
-
 /* All registers are word-sized, except for the configuration register.
    LM75 uses a high-byte first convention, which is exactly opposite to
    the usual practice. */

@@ -219,7 +214,7 @@
 	if (reg = LM75_REG_CONF)
 		return i2c_smbus_read_byte_data(client, reg);
 	else
-		return swap_bytes(i2c_smbus_read_word_data(client, reg));
+		return swab16(i2c_smbus_read_word_data(client, reg));
 }
 
 /* All registers are word-sized, except for the configuration register.

@@ -230,8 +225,7 @@
 	if (reg = LM75_REG_CONF)
 		return i2c_smbus_write_byte_data(client, reg, value);
 	else
-		return i2c_smbus_write_word_data(client, reg,
-						 swap_bytes(value));
+		return i2c_smbus_write_word_data(client, reg, swab16(value));
 }
 
 static void lm75_init_client(struct i2c_client *client)
drivers/i2c/chips/w83781d.c renamed from drivers/i2c/chips.orig/w83781d.c +5 -12
diff -ruN linux-2.6.5-rc2/drivers/i2c/chips.orig/w83781d.c linux-2.6.5-rc2/drivers/i2c/chips/w83781d.c
@@ -275,11 +275,6 @@
 static struct w83781d_data *w83781d_update_device(struct device *dev);
 static void w83781d_init_client(struct i2c_client *client);
 
-static inline u16 swap_bytes(u16 val)
-{
-	return (val >> 8) | (val << 8);
-}
-
 static struct i2c_driver w83781d_driver = {
 	.owner = THIS_MODULE,
 	.name = "w83781d",

@@ -1407,20 +1402,17 @@
 			/* convert from ISA to LM75 I2C addresses */
 			switch (reg & 0xff) {
 			case 0x50:	/* TEMP */
-				res -				    swap_bytes(i2c_smbus_read_word_data(cl, 0));
+				res = swab16(i2c_smbus_read_word_data(cl, 0));
 				break;
 			case 0x52:	/* CONFIG */
 				res = i2c_smbus_read_byte_data(cl, 1);
 				break;
 			case 0x53:	/* HYST */
-				res -				    swap_bytes(i2c_smbus_read_word_data(cl, 2));
+				res = swab16(i2c_smbus_read_word_data(cl, 2));
 				break;
 			case 0x55:	/* OVER */
 			default:
-				res -				    swap_bytes(i2c_smbus_read_word_data(cl, 3));
+				res = swab16(i2c_smbus_read_word_data(cl, 3));
 				break;
 			}
 		}

@@ -1481,12 +1473,10 @@
 				i2c_smbus_write_byte_data(cl, 1, value & 0xff);
 				break;
 			case 0x53:	/* HYST */
-				i2c_smbus_write_word_data(cl, 2,
-							  swap_bytes(value));
+				i2c_smbus_write_word_data(cl, 2, swab16(value));
 				break;
 			case 0x55:	/* OVER */
-				i2c_smbus_write_word_data(cl, 3,
-							  swap_bytes(value));
+				i2c_smbus_write_word_data(cl, 3, swab16(value));
 				break;
 			}
 		}
Signature · 2 lines
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/
#3 lore raw

PATCH 1/1 Refactor swap_bytes in i2c chip drivers

In reply to Jean Delvare
Quoted 8 lines · Jean Delvare
Quoted 3 lines

Ick, no, we should be using the proper kernel call for this, swab16(), right? Care to redo this patch to just fix the drivers and get rid of our duplicating of this function.

Oh, I didn't know such a function existed, sorry.

Here's a new patch, hope you like it. Tested to work on my as99127f, btw (w83781d driver).

Looks good, applied, thanks.

greg k-h

author
#4 lore raw

PATCH 1/1 Refactor swap_bytes in i2c chip drivers

In reply to Jean Delvare
Quoted 10 lines
Quoted 8 lines
Quoted 3 lines

Ick, no, we should be using the proper kernel call for this, swab16(), right? Care to redo this patch to just fix the drivers and get rid of our duplicating of this function.

Oh, I didn't know such a function existed, sorry.

Here's a new patch, hope you like it. Tested to work on my as99127f, btw(w83781d driver).

Looks good, applied, thanks.

Hm, it doesn't show in linux-2.6.5-rc3-mm4, although all others patches you accepted the same day do.

I was planning to submit my next patch against this tree, but then it's likely to cause trouble. Can you please verify what happened to the swap_bytes patch?

Thanks.

Signature · 2 lines
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/
#5 lore raw

PATCH 1/1 Refactor swap_bytes in i2c chip drivers

In reply to Jean Delvare
Quoted 6 lines · Jean Delvare
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines

Ick, no, we should be using the proper kernel call for this, swab16(), right? Care to redo this patch to just fix the drivers and get rid of our duplicating of this function.

Oh, I didn't know such a function existed, sorry.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Here's a new patch, hope you like it. Tested to work on my as99127f, btw(w83781d driver).

Looks good, applied, thanks.

Hm, it doesn't show in linux-2.6.5-rc3-mm4, although all others patches you accepted the same day do.

Hm, odd. You are right, I just went and applied it again. Sorry about that, sometimes bk just doesn't apply the patch... Or I messed up with all of these different ones floating around :)

thanks,

greg k-h