This eliminates a 1-bit left shift in every single caller,
and also makes the inner loop of the CRC computation more efficient.
And purged #include <linux/crc7.h> from files that don't use it at all.
Signed-off-by: George Spelvin <[email protected]>
---
Since all of the affected drivers are officially orphaned, this is
being sent to a hodge-podge of people who touched the files recently.
Assuming the MMC people don't complain, is it okay to send this through
the wireless tree?
drivers/mmc/host/mmc_spi.c | 2 +-
drivers/net/wireless/ti/wl1251/acx.c | 1 -
drivers/net/wireless/ti/wl1251/cmd.c | 1 -
drivers/net/wireless/ti/wl1251/spi.c | 3 +-
drivers/net/wireless/ti/wlcore/spi.c | 3 +-
include/linux/crc7.h | 2 +-
lib/crc7.c | 74 ++++++++++++++++++++----------------
7 files changed, 45 insertions(+), 41 deletions(-)
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 0a87e56913..2c9e879c09 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -472,7 +472,7 @@ mmc_spi_command_send(struct mmc_spi_host *host,
*cp++ = (u8)(arg >> 16);
*cp++ = (u8)(arg >> 8);
*cp++ = (u8)arg;
- *cp++ = (crc7(0, &data->status[1], 5) << 1) | 0x01;
+ *cp++ = crc7(0, &data->status[1], 5) | 0x01;
/* Then, read up to 13 bytes (while writing all-ones):
* - N(CR) (== 1..8) bytes of all-ones
diff --git a/drivers/net/wireless/ti/wl1251/acx.c b/drivers/net/wireless/ti/wl1251/acx.c
index 5a4ec56c83..5695628757 100644
--- a/drivers/net/wireless/ti/wl1251/acx.c
+++ b/drivers/net/wireless/ti/wl1251/acx.c
@@ -2,7 +2,6 @@
#include <linux/module.h>
#include <linux/slab.h>
-#include <linux/crc7.h>
#include "wl1251.h"
#include "reg.h"
diff --git a/drivers/net/wireless/ti/wl1251/cmd.c b/drivers/net/wireless/ti/wl1251/cmd.c
index bf1fa18b97..ede31f048e 100644
--- a/drivers/net/wireless/ti/wl1251/cmd.c
+++ b/drivers/net/wireless/ti/wl1251/cmd.c
@@ -2,7 +2,6 @@
#include <linux/module.h>
#include <linux/slab.h>
-#include <linux/crc7.h>
#include <linux/etherdevice.h>
#include "wl1251.h"
diff --git a/drivers/net/wireless/ti/wl1251/spi.c b/drivers/net/wireless/ti/wl1251/spi.c
index b06d36d993..47875fc6be 100644
--- a/drivers/net/wireless/ti/wl1251/spi.c
+++ b/drivers/net/wireless/ti/wl1251/spi.c
@@ -122,8 +122,7 @@ static void wl1251_spi_wake(struct wl1251 *wl)
crc[3] = cmd[6];
crc[4] = cmd[5];
- cmd[4] |= crc7(0, crc, WSPI_INIT_CMD_CRC_LEN) << 1;
- cmd[4] |= WSPI_INIT_CMD_END;
+ cmd[4] = crc7(0, crc, WSPI_INIT_CMD_CRC_LEN) | WSPI_INIT_CMD_END;
t.tx_buf = cmd;
t.len = WSPI_INIT_CMD_LEN;
diff --git a/drivers/net/wireless/ti/wlcore/spi.c b/drivers/net/wireless/ti/wlcore/spi.c
index dbe826dd7c..8b3e38e453 100644
--- a/drivers/net/wireless/ti/wlcore/spi.c
+++ b/drivers/net/wireless/ti/wlcore/spi.c
@@ -151,8 +151,7 @@ static void wl12xx_spi_init(struct device *child)
crc[3] = cmd[6];
crc[4] = cmd[5];
- cmd[4] |= crc7(0, crc, WSPI_INIT_CMD_CRC_LEN) << 1;
- cmd[4] |= WSPI_INIT_CMD_END;
+ cmd[4] = crc7(0, crc, WSPI_INIT_CMD_CRC_LEN) | WSPI_INIT_CMD_END;
t.tx_buf = cmd;
t.len = WSPI_INIT_CMD_LEN;
diff --git a/include/linux/crc7.h b/include/linux/crc7.h
index 1786e772d5..d2155dc953 100644
--- a/include/linux/crc7.h
+++ b/include/linux/crc7.h
@@ -6,7 +6,7 @@ extern const u8 crc7_syndrome_table[256];
static inline u8 crc7_byte(u8 crc, u8 data)
{
- return crc7_syndrome_table[(crc << 1) ^ data];
+ return crc7_syndrome_table[crc ^ data];
}
extern u8 crc7(u8 crc, const u8 *buffer, size_t len);
diff --git a/lib/crc7.c b/lib/crc7.c
index f1c3a144ce..4a73b26e7a 100644
--- a/lib/crc7.c
+++ b/lib/crc7.c
@@ -10,40 +10,45 @@
#include <linux/crc7.h>
-/* Table for CRC-7 (polynomial x^7 + x^3 + 1) */
+/*
+ * Table for CRC-7 (polynomial x^7 + x^3 + 1).
+ * This is a big-endian CRC (msbit is highest power of x),
+ * aligned so the msbit of the byte is the x^6 coefficient
+ * and the lsbit is not used.
+ */
const u8 crc7_syndrome_table[256] = {
- 0x00, 0x09, 0x12, 0x1b, 0x24, 0x2d, 0x36, 0x3f,
- 0x48, 0x41, 0x5a, 0x53, 0x6c, 0x65, 0x7e, 0x77,
- 0x19, 0x10, 0x0b, 0x02, 0x3d, 0x34, 0x2f, 0x26,
- 0x51, 0x58, 0x43, 0x4a, 0x75, 0x7c, 0x67, 0x6e,
- 0x32, 0x3b, 0x20, 0x29, 0x16, 0x1f, 0x04, 0x0d,
- 0x7a, 0x73, 0x68, 0x61, 0x5e, 0x57, 0x4c, 0x45,
- 0x2b, 0x22, 0x39, 0x30, 0x0f, 0x06, 0x1d, 0x14,
- 0x63, 0x6a, 0x71, 0x78, 0x47, 0x4e, 0x55, 0x5c,
- 0x64, 0x6d, 0x76, 0x7f, 0x40, 0x49, 0x52, 0x5b,
- 0x2c, 0x25, 0x3e, 0x37, 0x08, 0x01, 0x1a, 0x13,
- 0x7d, 0x74, 0x6f, 0x66, 0x59, 0x50, 0x4b, 0x42,
- 0x35, 0x3c, 0x27, 0x2e, 0x11, 0x18, 0x03, 0x0a,
- 0x56, 0x5f, 0x44, 0x4d, 0x72, 0x7b, 0x60, 0x69,
- 0x1e, 0x17, 0x0c, 0x05, 0x3a, 0x33, 0x28, 0x21,
- 0x4f, 0x46, 0x5d, 0x54, 0x6b, 0x62, 0x79, 0x70,
- 0x07, 0x0e, 0x15, 0x1c, 0x23, 0x2a, 0x31, 0x38,
- 0x41, 0x48, 0x53, 0x5a, 0x65, 0x6c, 0x77, 0x7e,
- 0x09, 0x00, 0x1b, 0x12, 0x2d, 0x24, 0x3f, 0x36,
- 0x58, 0x51, 0x4a, 0x43, 0x7c, 0x75, 0x6e, 0x67,
- 0x10, 0x19, 0x02, 0x0b, 0x34, 0x3d, 0x26, 0x2f,
- 0x73, 0x7a, 0x61, 0x68, 0x57, 0x5e, 0x45, 0x4c,
- 0x3b, 0x32, 0x29, 0x20, 0x1f, 0x16, 0x0d, 0x04,
- 0x6a, 0x63, 0x78, 0x71, 0x4e, 0x47, 0x5c, 0x55,
- 0x22, 0x2b, 0x30, 0x39, 0x06, 0x0f, 0x14, 0x1d,
- 0x25, 0x2c, 0x37, 0x3e, 0x01, 0x08, 0x13, 0x1a,
- 0x6d, 0x64, 0x7f, 0x76, 0x49, 0x40, 0x5b, 0x52,
- 0x3c, 0x35, 0x2e, 0x27, 0x18, 0x11, 0x0a, 0x03,
- 0x74, 0x7d, 0x66, 0x6f, 0x50, 0x59, 0x42, 0x4b,
- 0x17, 0x1e, 0x05, 0x0c, 0x33, 0x3a, 0x21, 0x28,
- 0x5f, 0x56, 0x4d, 0x44, 0x7b, 0x72, 0x69, 0x60,
- 0x0e, 0x07, 0x1c, 0x15, 0x2a, 0x23, 0x38, 0x31,
- 0x46, 0x4f, 0x54, 0x5d, 0x62, 0x6b, 0x70, 0x79
+ 0x00, 0x12, 0x24, 0x36, 0x48, 0x5a, 0x6c, 0x7e,
+ 0x90, 0x82, 0xb4, 0xa6, 0xd8, 0xca, 0xfc, 0xee,
+ 0x32, 0x20, 0x16, 0x04, 0x7a, 0x68, 0x5e, 0x4c,
+ 0xa2, 0xb0, 0x86, 0x94, 0xea, 0xf8, 0xce, 0xdc,
+ 0x64, 0x76, 0x40, 0x52, 0x2c, 0x3e, 0x08, 0x1a,
+ 0xf4, 0xe6, 0xd0, 0xc2, 0xbc, 0xae, 0x98, 0x8a,
+ 0x56, 0x44, 0x72, 0x60, 0x1e, 0x0c, 0x3a, 0x28,
+ 0xc6, 0xd4, 0xe2, 0xf0, 0x8e, 0x9c, 0xaa, 0xb8,
+ 0xc8, 0xda, 0xec, 0xfe, 0x80, 0x92, 0xa4, 0xb6,
+ 0x58, 0x4a, 0x7c, 0x6e, 0x10, 0x02, 0x34, 0x26,
+ 0xfa, 0xe8, 0xde, 0xcc, 0xb2, 0xa0, 0x96, 0x84,
+ 0x6a, 0x78, 0x4e, 0x5c, 0x22, 0x30, 0x06, 0x14,
+ 0xac, 0xbe, 0x88, 0x9a, 0xe4, 0xf6, 0xc0, 0xd2,
+ 0x3c, 0x2e, 0x18, 0x0a, 0x74, 0x66, 0x50, 0x42,
+ 0x9e, 0x8c, 0xba, 0xa8, 0xd6, 0xc4, 0xf2, 0xe0,
+ 0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54, 0x62, 0x70,
+ 0x82, 0x90, 0xa6, 0xb4, 0xca, 0xd8, 0xee, 0xfc,
+ 0x12, 0x00, 0x36, 0x24, 0x5a, 0x48, 0x7e, 0x6c,
+ 0xb0, 0xa2, 0x94, 0x86, 0xf8, 0xea, 0xdc, 0xce,
+ 0x20, 0x32, 0x04, 0x16, 0x68, 0x7a, 0x4c, 0x5e,
+ 0xe6, 0xf4, 0xc2, 0xd0, 0xae, 0xbc, 0x8a, 0x98,
+ 0x76, 0x64, 0x52, 0x40, 0x3e, 0x2c, 0x1a, 0x08,
+ 0xd4, 0xc6, 0xf0, 0xe2, 0x9c, 0x8e, 0xb8, 0xaa,
+ 0x44, 0x56, 0x60, 0x72, 0x0c, 0x1e, 0x28, 0x3a,
+ 0x4a, 0x58, 0x6e, 0x7c, 0x02, 0x10, 0x26, 0x34,
+ 0xda, 0xc8, 0xfe, 0xec, 0x92, 0x80, 0xb6, 0xa4,
+ 0x78, 0x6a, 0x5c, 0x4e, 0x30, 0x22, 0x14, 0x06,
+ 0xe8, 0xfa, 0xcc, 0xde, 0xa0, 0xb2, 0x84, 0x96,
+ 0x2e, 0x3c, 0x0a, 0x18, 0x66, 0x74, 0x42, 0x50,
+ 0xbe, 0xac, 0x9a, 0x88, 0xf6, 0xe4, 0xd2, 0xc0,
+ 0x1c, 0x0e, 0x38, 0x2a, 0x54, 0x46, 0x70, 0x62,
+ 0x8c, 0x9e, 0xa8, 0xba, 0xc4, 0xd6, 0xe0, 0xf2
};
EXPORT_SYMBOL(crc7_syndrome_table);
@@ -55,6 +60,9 @@ EXPORT_SYMBOL(crc7_syndrome_table);
* Context: any
*
* Returns the updated CRC7 value.
+ * The CRC7 is left-aligned in the byte (the lsbit is always 0), as that
+ * makes the computation easier, and all callers want it in that form.
+ *
*/
u8 crc7(u8 crc, const u8 *buffer, size_t len)
{
--
1.9.2
On Wed 2014-05-14 20:32:25, George Spelvin wrote:
> Pavel Machek wrote;
> > On Sun 2014-05-11 05:16:07, George Spelvin wrote:
> >> To do it properly, I have to rename all of:
> >>
> >> crc7_syndrome_table[]
> >> crc7_byte()
> >> crc7()
> >>
> >> even though the third is the only (in-tree) user of the first two.
>
> > If the first two are static, there's no problem.
>
> They're not; they're exported from the header (even though, as
> I mentioned, their only user is crc7()).
>
> So my patch v2 1/3 renamed all three.
That's one way.
Other would be to make them static so people can't use old interfaces
by mistake.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>From 770aa22e6c9c92027e3e21797192ccabb3e7c70e Mon Sep 17 00:00:00 2001
From: George Spelvin <[email protected]>
Date: Sat, 10 May 2014 10:32:57 -0400
Subject: [PATCH v2 1/3] lib/crc7: Shift crc7() output left 1 bit
This eliminates a 1-bit left shift in every single caller,
and makes the inner loop of the CRC computation more efficient.
Renamed crc7 to crc7_be (big-endian) since the interface changed.
Also purged #include <linux/crc7.h> from files that don't use it at all.
Signed-off-by: George Spelvin <[email protected]>
---
v2: Functions renamed to reflect different results; Pavel Machek
prompted me to think of something not too ugly.
Since all of the affected drivers are officially orphaned, this is
being sent to a hodge-podge of people who touched the files recently.
Assuming the MMC people don't complain, is it okay to send this through
the wireless tree?
drivers/mmc/host/mmc_spi.c | 2 +-
drivers/net/wireless/ti/wl1251/acx.c | 1 -
drivers/net/wireless/ti/wl1251/cmd.c | 1 -
drivers/net/wireless/ti/wl1251/spi.c | 3 +-
drivers/net/wireless/ti/wlcore/spi.c | 3 +-
include/linux/crc7.h | 8 ++--
lib/crc7.c | 84 ++++++++++++++++++++----------------
7 files changed, 53 insertions(+), 49 deletions(-)
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 0a87e56913..338e2202ea 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -472,7 +472,7 @@ mmc_spi_command_send(struct mmc_spi_host *host,
*cp++ = (u8)(arg >> 16);
*cp++ = (u8)(arg >> 8);
*cp++ = (u8)arg;
- *cp++ = (crc7(0, &data->status[1], 5) << 1) | 0x01;
+ *cp++ = crc7_be(0, &data->status[1], 5) | 0x01;
/* Then, read up to 13 bytes (while writing all-ones):
* - N(CR) (== 1..8) bytes of all-ones
diff --git a/drivers/net/wireless/ti/wl1251/acx.c b/drivers/net/wireless/ti/wl1251/acx.c
index 5a4ec56c83..5695628757 100644
--- a/drivers/net/wireless/ti/wl1251/acx.c
+++ b/drivers/net/wireless/ti/wl1251/acx.c
@@ -2,7 +2,6 @@
#include <linux/module.h>
#include <linux/slab.h>
-#include <linux/crc7.h>
#include "wl1251.h"
#include "reg.h"
diff --git a/drivers/net/wireless/ti/wl1251/cmd.c b/drivers/net/wireless/ti/wl1251/cmd.c
index bf1fa18b97..ede31f048e 100644
--- a/drivers/net/wireless/ti/wl1251/cmd.c
+++ b/drivers/net/wireless/ti/wl1251/cmd.c
@@ -2,7 +2,6 @@
#include <linux/module.h>
#include <linux/slab.h>
-#include <linux/crc7.h>
#include <linux/etherdevice.h>
#include "wl1251.h"
diff --git a/drivers/net/wireless/ti/wl1251/spi.c b/drivers/net/wireless/ti/wl1251/spi.c
index b06d36d993..e94b57cd5a 100644
--- a/drivers/net/wireless/ti/wl1251/spi.c
+++ b/drivers/net/wireless/ti/wl1251/spi.c
@@ -122,8 +122,7 @@ static void wl1251_spi_wake(struct wl1251 *wl)
crc[3] = cmd[6];
crc[4] = cmd[5];
- cmd[4] |= crc7(0, crc, WSPI_INIT_CMD_CRC_LEN) << 1;
- cmd[4] |= WSPI_INIT_CMD_END;
+ cmd[4] = crc7_be(0, crc, WSPI_INIT_CMD_CRC_LEN) | WSPI_INIT_CMD_END;
t.tx_buf = cmd;
t.len = WSPI_INIT_CMD_LEN;
diff --git a/drivers/net/wireless/ti/wlcore/spi.c b/drivers/net/wireless/ti/wlcore/spi.c
index dbe826dd7c..0497353c4a 100644
--- a/drivers/net/wireless/ti/wlcore/spi.c
+++ b/drivers/net/wireless/ti/wlcore/spi.c
@@ -151,8 +151,7 @@ static void wl12xx_spi_init(struct device *child)
crc[3] = cmd[6];
crc[4] = cmd[5];
- cmd[4] |= crc7(0, crc, WSPI_INIT_CMD_CRC_LEN) << 1;
- cmd[4] |= WSPI_INIT_CMD_END;
+ cmd[4] = crc7_be(0, crc, WSPI_INIT_CMD_CRC_LEN) | WSPI_INIT_CMD_END;
t.tx_buf = cmd;
t.len = WSPI_INIT_CMD_LEN;
diff --git a/include/linux/crc7.h b/include/linux/crc7.h
index 1786e772d5..d590765106 100644
--- a/include/linux/crc7.h
+++ b/include/linux/crc7.h
@@ -2,13 +2,13 @@
#define _LINUX_CRC7_H
#include <linux/types.h>
-extern const u8 crc7_syndrome_table[256];
+extern const u8 crc7_be_syndrome_table[256];
-static inline u8 crc7_byte(u8 crc, u8 data)
+static inline u8 crc7_be_byte(u8 crc, u8 data)
{
- return crc7_syndrome_table[(crc << 1) ^ data];
+ return crc7_be_syndrome_table[crc ^ data];
}
-extern u8 crc7(u8 crc, const u8 *buffer, size_t len);
+extern u8 crc7_be(u8 crc, const u8 *buffer, size_t len);
#endif
diff --git a/lib/crc7.c b/lib/crc7.c
index f1c3a144ce..bf6255e239 100644
--- a/lib/crc7.c
+++ b/lib/crc7.c
@@ -10,42 +10,47 @@
#include <linux/crc7.h>
-/* Table for CRC-7 (polynomial x^7 + x^3 + 1) */
-const u8 crc7_syndrome_table[256] = {
- 0x00, 0x09, 0x12, 0x1b, 0x24, 0x2d, 0x36, 0x3f,
- 0x48, 0x41, 0x5a, 0x53, 0x6c, 0x65, 0x7e, 0x77,
- 0x19, 0x10, 0x0b, 0x02, 0x3d, 0x34, 0x2f, 0x26,
- 0x51, 0x58, 0x43, 0x4a, 0x75, 0x7c, 0x67, 0x6e,
- 0x32, 0x3b, 0x20, 0x29, 0x16, 0x1f, 0x04, 0x0d,
- 0x7a, 0x73, 0x68, 0x61, 0x5e, 0x57, 0x4c, 0x45,
- 0x2b, 0x22, 0x39, 0x30, 0x0f, 0x06, 0x1d, 0x14,
- 0x63, 0x6a, 0x71, 0x78, 0x47, 0x4e, 0x55, 0x5c,
- 0x64, 0x6d, 0x76, 0x7f, 0x40, 0x49, 0x52, 0x5b,
- 0x2c, 0x25, 0x3e, 0x37, 0x08, 0x01, 0x1a, 0x13,
- 0x7d, 0x74, 0x6f, 0x66, 0x59, 0x50, 0x4b, 0x42,
- 0x35, 0x3c, 0x27, 0x2e, 0x11, 0x18, 0x03, 0x0a,
- 0x56, 0x5f, 0x44, 0x4d, 0x72, 0x7b, 0x60, 0x69,
- 0x1e, 0x17, 0x0c, 0x05, 0x3a, 0x33, 0x28, 0x21,
- 0x4f, 0x46, 0x5d, 0x54, 0x6b, 0x62, 0x79, 0x70,
- 0x07, 0x0e, 0x15, 0x1c, 0x23, 0x2a, 0x31, 0x38,
- 0x41, 0x48, 0x53, 0x5a, 0x65, 0x6c, 0x77, 0x7e,
- 0x09, 0x00, 0x1b, 0x12, 0x2d, 0x24, 0x3f, 0x36,
- 0x58, 0x51, 0x4a, 0x43, 0x7c, 0x75, 0x6e, 0x67,
- 0x10, 0x19, 0x02, 0x0b, 0x34, 0x3d, 0x26, 0x2f,
- 0x73, 0x7a, 0x61, 0x68, 0x57, 0x5e, 0x45, 0x4c,
- 0x3b, 0x32, 0x29, 0x20, 0x1f, 0x16, 0x0d, 0x04,
- 0x6a, 0x63, 0x78, 0x71, 0x4e, 0x47, 0x5c, 0x55,
- 0x22, 0x2b, 0x30, 0x39, 0x06, 0x0f, 0x14, 0x1d,
- 0x25, 0x2c, 0x37, 0x3e, 0x01, 0x08, 0x13, 0x1a,
- 0x6d, 0x64, 0x7f, 0x76, 0x49, 0x40, 0x5b, 0x52,
- 0x3c, 0x35, 0x2e, 0x27, 0x18, 0x11, 0x0a, 0x03,
- 0x74, 0x7d, 0x66, 0x6f, 0x50, 0x59, 0x42, 0x4b,
- 0x17, 0x1e, 0x05, 0x0c, 0x33, 0x3a, 0x21, 0x28,
- 0x5f, 0x56, 0x4d, 0x44, 0x7b, 0x72, 0x69, 0x60,
- 0x0e, 0x07, 0x1c, 0x15, 0x2a, 0x23, 0x38, 0x31,
- 0x46, 0x4f, 0x54, 0x5d, 0x62, 0x6b, 0x70, 0x79
+/*
+ * Table for CRC-7 (polynomial x^7 + x^3 + 1).
+ * This is a big-endian CRC (msbit is highest power of x),
+ * aligned so the msbit of the byte is the x^6 coefficient
+ * and the lsbit is not used.
+ */
+const u8 crc7_be_syndrome_table[256] = {
+ 0x00, 0x12, 0x24, 0x36, 0x48, 0x5a, 0x6c, 0x7e,
+ 0x90, 0x82, 0xb4, 0xa6, 0xd8, 0xca, 0xfc, 0xee,
+ 0x32, 0x20, 0x16, 0x04, 0x7a, 0x68, 0x5e, 0x4c,
+ 0xa2, 0xb0, 0x86, 0x94, 0xea, 0xf8, 0xce, 0xdc,
+ 0x64, 0x76, 0x40, 0x52, 0x2c, 0x3e, 0x08, 0x1a,
+ 0xf4, 0xe6, 0xd0, 0xc2, 0xbc, 0xae, 0x98, 0x8a,
+ 0x56, 0x44, 0x72, 0x60, 0x1e, 0x0c, 0x3a, 0x28,
+ 0xc6, 0xd4, 0xe2, 0xf0, 0x8e, 0x9c, 0xaa, 0xb8,
+ 0xc8, 0xda, 0xec, 0xfe, 0x80, 0x92, 0xa4, 0xb6,
+ 0x58, 0x4a, 0x7c, 0x6e, 0x10, 0x02, 0x34, 0x26,
+ 0xfa, 0xe8, 0xde, 0xcc, 0xb2, 0xa0, 0x96, 0x84,
+ 0x6a, 0x78, 0x4e, 0x5c, 0x22, 0x30, 0x06, 0x14,
+ 0xac, 0xbe, 0x88, 0x9a, 0xe4, 0xf6, 0xc0, 0xd2,
+ 0x3c, 0x2e, 0x18, 0x0a, 0x74, 0x66, 0x50, 0x42,
+ 0x9e, 0x8c, 0xba, 0xa8, 0xd6, 0xc4, 0xf2, 0xe0,
+ 0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54, 0x62, 0x70,
+ 0x82, 0x90, 0xa6, 0xb4, 0xca, 0xd8, 0xee, 0xfc,
+ 0x12, 0x00, 0x36, 0x24, 0x5a, 0x48, 0x7e, 0x6c,
+ 0xb0, 0xa2, 0x94, 0x86, 0xf8, 0xea, 0xdc, 0xce,
+ 0x20, 0x32, 0x04, 0x16, 0x68, 0x7a, 0x4c, 0x5e,
+ 0xe6, 0xf4, 0xc2, 0xd0, 0xae, 0xbc, 0x8a, 0x98,
+ 0x76, 0x64, 0x52, 0x40, 0x3e, 0x2c, 0x1a, 0x08,
+ 0xd4, 0xc6, 0xf0, 0xe2, 0x9c, 0x8e, 0xb8, 0xaa,
+ 0x44, 0x56, 0x60, 0x72, 0x0c, 0x1e, 0x28, 0x3a,
+ 0x4a, 0x58, 0x6e, 0x7c, 0x02, 0x10, 0x26, 0x34,
+ 0xda, 0xc8, 0xfe, 0xec, 0x92, 0x80, 0xb6, 0xa4,
+ 0x78, 0x6a, 0x5c, 0x4e, 0x30, 0x22, 0x14, 0x06,
+ 0xe8, 0xfa, 0xcc, 0xde, 0xa0, 0xb2, 0x84, 0x96,
+ 0x2e, 0x3c, 0x0a, 0x18, 0x66, 0x74, 0x42, 0x50,
+ 0xbe, 0xac, 0x9a, 0x88, 0xf6, 0xe4, 0xd2, 0xc0,
+ 0x1c, 0x0e, 0x38, 0x2a, 0x54, 0x46, 0x70, 0x62,
+ 0x8c, 0x9e, 0xa8, 0xba, 0xc4, 0xd6, 0xe0, 0xf2
};
-EXPORT_SYMBOL(crc7_syndrome_table);
+EXPORT_SYMBOL(crc7_be_syndrome_table);
/**
* crc7 - update the CRC7 for the data buffer
@@ -55,14 +60,17 @@ EXPORT_SYMBOL(crc7_syndrome_table);
* Context: any
*
* Returns the updated CRC7 value.
+ * The CRC7 is left-aligned in the byte (the lsbit is always 0), as that
+ * makes the computation easier, and all callers want it in that form.
+ *
*/
-u8 crc7(u8 crc, const u8 *buffer, size_t len)
+u8 crc7_be(u8 crc, const u8 *buffer, size_t len)
{
while (len--)
- crc = crc7_byte(crc, *buffer++);
+ crc = crc7_be_byte(crc, *buffer++);
return crc;
}
-EXPORT_SYMBOL(crc7);
+EXPORT_SYMBOL(crc7_be);
MODULE_DESCRIPTION("CRC7 calculations");
MODULE_LICENSE("GPL");
--
1.9.2
These devices require commands stored in buffers in an odd order,
different from that in which the CRC is computed.
Rather than make two copies of the commands in two different orders,
form the commands in logical (CRC) order, append the CRC, then byte-swap
in place to the desired order.
The old code worked fine, I'm just scratching an "ugh, that's ugly"
itch.
Signed-off-by: George Spelvin <[email protected]>
---
I spotted this while making the previous crc7 change.
This looks straightforward, but I don't actually have either of these devices to test.
At least one other careful desk-check is solicited.
drivers/net/wireless/ti/wl1251/spi.c | 43 +++++++++++++++++-----------------
drivers/net/wireless/ti/wlcore/spi.c | 45 ++++++++++++++++++------------------
2 files changed, 43 insertions(+), 45 deletions(-)
diff --git a/drivers/net/wireless/ti/wl1251/spi.c b/drivers/net/wireless/ti/wl1251/spi.c
index e94b57cd5a..a0aa8fa723 100644
--- a/drivers/net/wireless/ti/wl1251/spi.c
+++ b/drivers/net/wireless/ti/wl1251/spi.c
@@ -23,6 +23,7 @@
#include <linux/irq.h>
#include <linux/module.h>
#include <linux/slab.h>
+#include <linux/swab.h>
#include <linux/crc7.h>
#include <linux/spi/spi.h>
#include <linux/wl12xx.h>
@@ -83,46 +84,44 @@ static void wl1251_spi_reset(struct wl1251 *wl)
static void wl1251_spi_wake(struct wl1251 *wl)
{
- u8 crc[WSPI_INIT_CMD_CRC_LEN], *cmd;
struct spi_transfer t;
struct spi_message m;
+ u8 *cmd = kzalloc(WSPI_INIT_CMD_LEN, GFP_KERNEL);
- cmd = kzalloc(WSPI_INIT_CMD_LEN, GFP_KERNEL);
if (!cmd) {
wl1251_error("could not allocate cmd for spi init");
return;
}
- memset(crc, 0, sizeof(crc));
memset(&t, 0, sizeof(t));
spi_message_init(&m);
/* Set WSPI_INIT_COMMAND
* the data is being send from the MSB to LSB
*/
- cmd[2] = 0xff;
- cmd[3] = 0xff;
- cmd[1] = WSPI_INIT_CMD_START | WSPI_INIT_CMD_TX;
- cmd[0] = 0;
- cmd[7] = 0;
- cmd[6] |= HW_ACCESS_WSPI_INIT_CMD_MASK << 3;
- cmd[6] |= HW_ACCESS_WSPI_FIXED_BUSY_LEN & WSPI_INIT_CMD_FIXEDBUSY_LEN;
+ cmd[0] = 0xff;
+ cmd[1] = 0xff;
+ cmd[2] = WSPI_INIT_CMD_START | WSPI_INIT_CMD_TX;
+ cmd[3] = 0;
+ cmd[4] = 0;
+ cmd[5] = HW_ACCESS_WSPI_INIT_CMD_MASK << 3;
+ cmd[5] |= HW_ACCESS_WSPI_FIXED_BUSY_LEN & WSPI_INIT_CMD_FIXEDBUSY_LEN;
+
+ cmd[6] = WSPI_INIT_CMD_IOD | WSPI_INIT_CMD_IP | WSPI_INIT_CMD_CS
+ | WSPI_INIT_CMD_WSPI | WSPI_INIT_CMD_WS;
if (HW_ACCESS_WSPI_FIXED_BUSY_LEN == 0)
- cmd[5] |= WSPI_INIT_CMD_DIS_FIXEDBUSY;
+ cmd[6] |= WSPI_INIT_CMD_DIS_FIXEDBUSY;
else
- cmd[5] |= WSPI_INIT_CMD_EN_FIXEDBUSY;
-
- cmd[5] |= WSPI_INIT_CMD_IOD | WSPI_INIT_CMD_IP | WSPI_INIT_CMD_CS
- | WSPI_INIT_CMD_WSPI | WSPI_INIT_CMD_WS;
-
- crc[0] = cmd[1];
- crc[1] = cmd[0];
- crc[2] = cmd[7];
- crc[3] = cmd[6];
- crc[4] = cmd[5];
+ cmd[6] |= WSPI_INIT_CMD_EN_FIXEDBUSY;
- cmd[4] = crc7_be(0, crc, WSPI_INIT_CMD_CRC_LEN) | WSPI_INIT_CMD_END;
+ cmd[7] = crc7_be(0, cmd+2, WSPI_INIT_CMD_CRC_LEN) | WSPI_INIT_CMD_END;
+ /*
+ * The above is the logical order; it must actually be stored
+ * in the buffer byte-swapped.
+ */
+ __swab32s((u32 *)cmd);
+ __swab32s((u32 *)cmd+1);
t.tx_buf = cmd;
t.len = WSPI_INIT_CMD_LEN;
diff --git a/drivers/net/wireless/ti/wlcore/spi.c b/drivers/net/wireless/ti/wlcore/spi.c
index 0497353c4a..955861af35 100644
--- a/drivers/net/wireless/ti/wlcore/spi.c
+++ b/drivers/net/wireless/ti/wlcore/spi.c
@@ -24,11 +24,12 @@
#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/swab.h>
#include <linux/crc7.h>
#include <linux/spi/spi.h>
#include <linux/wl12xx.h>
#include <linux/platform_device.h>
-#include <linux/slab.h>
#include "wlcore.h"
#include "wl12xx_80211.h"
@@ -110,18 +111,16 @@ static void wl12xx_spi_reset(struct device *child)
static void wl12xx_spi_init(struct device *child)
{
struct wl12xx_spi_glue *glue = dev_get_drvdata(child->parent);
- u8 crc[WSPI_INIT_CMD_CRC_LEN], *cmd;
struct spi_transfer t;
struct spi_message m;
+ u8 *cmd = kzalloc(WSPI_INIT_CMD_LEN, GFP_KERNEL);
- cmd = kzalloc(WSPI_INIT_CMD_LEN, GFP_KERNEL);
if (!cmd) {
dev_err(child->parent,
"could not allocate cmd for spi init\n");
return;
}
- memset(crc, 0, sizeof(crc));
memset(&t, 0, sizeof(t));
spi_message_init(&m);
@@ -129,29 +128,29 @@ static void wl12xx_spi_init(struct device *child)
* Set WSPI_INIT_COMMAND
* the data is being send from the MSB to LSB
*/
- cmd[2] = 0xff;
- cmd[3] = 0xff;
- cmd[1] = WSPI_INIT_CMD_START | WSPI_INIT_CMD_TX;
- cmd[0] = 0;
- cmd[7] = 0;
- cmd[6] |= HW_ACCESS_WSPI_INIT_CMD_MASK << 3;
- cmd[6] |= HW_ACCESS_WSPI_FIXED_BUSY_LEN & WSPI_INIT_CMD_FIXEDBUSY_LEN;
+ cmd[0] = 0xff;
+ cmd[1] = 0xff;
+ cmd[2] = WSPI_INIT_CMD_START | WSPI_INIT_CMD_TX;
+ cmd[3] = 0;
+ cmd[4] = 0;
+ cmd[5] = HW_ACCESS_WSPI_INIT_CMD_MASK << 3;
+ cmd[5] |= HW_ACCESS_WSPI_FIXED_BUSY_LEN & WSPI_INIT_CMD_FIXEDBUSY_LEN;
+
+ cmd[6] = WSPI_INIT_CMD_IOD | WSPI_INIT_CMD_IP | WSPI_INIT_CMD_CS
+ | WSPI_INIT_CMD_WSPI | WSPI_INIT_CMD_WS;
if (HW_ACCESS_WSPI_FIXED_BUSY_LEN == 0)
- cmd[5] |= WSPI_INIT_CMD_DIS_FIXEDBUSY;
+ cmd[6] |= WSPI_INIT_CMD_DIS_FIXEDBUSY;
else
- cmd[5] |= WSPI_INIT_CMD_EN_FIXEDBUSY;
+ cmd[6] |= WSPI_INIT_CMD_EN_FIXEDBUSY;
- cmd[5] |= WSPI_INIT_CMD_IOD | WSPI_INIT_CMD_IP | WSPI_INIT_CMD_CS
- | WSPI_INIT_CMD_WSPI | WSPI_INIT_CMD_WS;
-
- crc[0] = cmd[1];
- crc[1] = cmd[0];
- crc[2] = cmd[7];
- crc[3] = cmd[6];
- crc[4] = cmd[5];
-
- cmd[4] = crc7_be(0, crc, WSPI_INIT_CMD_CRC_LEN) | WSPI_INIT_CMD_END;
+ cmd[7] = crc7_be(0, cmd+2, WSPI_INIT_CMD_CRC_LEN) | WSPI_INIT_CMD_END;
+ /*
+ * The above is the logical order; it must actually be stored
+ * in the buffer byte-swapped.
+ */
+ __swab32s((u32 *)cmd);
+ __swab32s((u32 *)cmd+1);
t.tx_buf = cmd;
t.len = WSPI_INIT_CMD_LEN;
--
1.9.2
On Sun, May 11, 2014 at 12:05 PM, George Spelvin <[email protected]> wrote:
> Very minor source and binary size reduction.
>
> Signed-off-by: George Spelvin <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 11 May 2014 12:02, George Spelvin <[email protected]> wrote:
> From 770aa22e6c9c92027e3e21797192ccabb3e7c70e Mon Sep 17 00:00:00 2001
> From: George Spelvin <[email protected]>
> Date: Sat, 10 May 2014 10:32:57 -0400
> Subject: [PATCH v2 1/3] lib/crc7: Shift crc7() output left 1 bit
>
> This eliminates a 1-bit left shift in every single caller,
> and makes the inner loop of the CRC computation more efficient.
>
> Renamed crc7 to crc7_be (big-endian) since the interface changed.
>
> Also purged #include <linux/crc7.h> from files that don't use it at all.
>
> Signed-off-by: George Spelvin <[email protected]>
> ---
> v2: Functions renamed to reflect different results; Pavel Machek
> prompted me to think of something not too ugly.
>
> Since all of the affected drivers are officially orphaned, this is
> being sent to a hodge-podge of people who touched the files recently.
> Assuming the MMC people don't complain, is it okay to send this through
> the wireless tree?
Acked-by: Ulf Hansson <[email protected]>
Feel free to take it through the wireless tree.
Kind regards
Ulf Hansson
>
> drivers/mmc/host/mmc_spi.c | 2 +-
> drivers/net/wireless/ti/wl1251/acx.c | 1 -
> drivers/net/wireless/ti/wl1251/cmd.c | 1 -
> drivers/net/wireless/ti/wl1251/spi.c | 3 +-
> drivers/net/wireless/ti/wlcore/spi.c | 3 +-
> include/linux/crc7.h | 8 ++--
> lib/crc7.c | 84 ++++++++++++++++++++----------------
> 7 files changed, 53 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> index 0a87e56913..338e2202ea 100644
> --- a/drivers/mmc/host/mmc_spi.c
> +++ b/drivers/mmc/host/mmc_spi.c
> @@ -472,7 +472,7 @@ mmc_spi_command_send(struct mmc_spi_host *host,
> *cp++ = (u8)(arg >> 16);
> *cp++ = (u8)(arg >> 8);
> *cp++ = (u8)arg;
> - *cp++ = (crc7(0, &data->status[1], 5) << 1) | 0x01;
> + *cp++ = crc7_be(0, &data->status[1], 5) | 0x01;
>
> /* Then, read up to 13 bytes (while writing all-ones):
> * - N(CR) (== 1..8) bytes of all-ones
> diff --git a/drivers/net/wireless/ti/wl1251/acx.c b/drivers/net/wireless/ti/wl1251/acx.c
> index 5a4ec56c83..5695628757 100644
> --- a/drivers/net/wireless/ti/wl1251/acx.c
> +++ b/drivers/net/wireless/ti/wl1251/acx.c
> @@ -2,7 +2,6 @@
>
> #include <linux/module.h>
> #include <linux/slab.h>
> -#include <linux/crc7.h>
>
> #include "wl1251.h"
> #include "reg.h"
> diff --git a/drivers/net/wireless/ti/wl1251/cmd.c b/drivers/net/wireless/ti/wl1251/cmd.c
> index bf1fa18b97..ede31f048e 100644
> --- a/drivers/net/wireless/ti/wl1251/cmd.c
> +++ b/drivers/net/wireless/ti/wl1251/cmd.c
> @@ -2,7 +2,6 @@
>
> #include <linux/module.h>
> #include <linux/slab.h>
> -#include <linux/crc7.h>
> #include <linux/etherdevice.h>
>
> #include "wl1251.h"
> diff --git a/drivers/net/wireless/ti/wl1251/spi.c b/drivers/net/wireless/ti/wl1251/spi.c
> index b06d36d993..e94b57cd5a 100644
> --- a/drivers/net/wireless/ti/wl1251/spi.c
> +++ b/drivers/net/wireless/ti/wl1251/spi.c
> @@ -122,8 +122,7 @@ static void wl1251_spi_wake(struct wl1251 *wl)
> crc[3] = cmd[6];
> crc[4] = cmd[5];
>
> - cmd[4] |= crc7(0, crc, WSPI_INIT_CMD_CRC_LEN) << 1;
> - cmd[4] |= WSPI_INIT_CMD_END;
> + cmd[4] = crc7_be(0, crc, WSPI_INIT_CMD_CRC_LEN) | WSPI_INIT_CMD_END;
>
> t.tx_buf = cmd;
> t.len = WSPI_INIT_CMD_LEN;
> diff --git a/drivers/net/wireless/ti/wlcore/spi.c b/drivers/net/wireless/ti/wlcore/spi.c
> index dbe826dd7c..0497353c4a 100644
> --- a/drivers/net/wireless/ti/wlcore/spi.c
> +++ b/drivers/net/wireless/ti/wlcore/spi.c
> @@ -151,8 +151,7 @@ static void wl12xx_spi_init(struct device *child)
> crc[3] = cmd[6];
> crc[4] = cmd[5];
>
> - cmd[4] |= crc7(0, crc, WSPI_INIT_CMD_CRC_LEN) << 1;
> - cmd[4] |= WSPI_INIT_CMD_END;
> + cmd[4] = crc7_be(0, crc, WSPI_INIT_CMD_CRC_LEN) | WSPI_INIT_CMD_END;
>
> t.tx_buf = cmd;
> t.len = WSPI_INIT_CMD_LEN;
> diff --git a/include/linux/crc7.h b/include/linux/crc7.h
> index 1786e772d5..d590765106 100644
> --- a/include/linux/crc7.h
> +++ b/include/linux/crc7.h
> @@ -2,13 +2,13 @@
> #define _LINUX_CRC7_H
> #include <linux/types.h>
>
> -extern const u8 crc7_syndrome_table[256];
> +extern const u8 crc7_be_syndrome_table[256];
>
> -static inline u8 crc7_byte(u8 crc, u8 data)
> +static inline u8 crc7_be_byte(u8 crc, u8 data)
> {
> - return crc7_syndrome_table[(crc << 1) ^ data];
> + return crc7_be_syndrome_table[crc ^ data];
> }
>
> -extern u8 crc7(u8 crc, const u8 *buffer, size_t len);
> +extern u8 crc7_be(u8 crc, const u8 *buffer, size_t len);
>
> #endif
> diff --git a/lib/crc7.c b/lib/crc7.c
> index f1c3a144ce..bf6255e239 100644
> --- a/lib/crc7.c
> +++ b/lib/crc7.c
> @@ -10,42 +10,47 @@
> #include <linux/crc7.h>
>
>
> -/* Table for CRC-7 (polynomial x^7 + x^3 + 1) */
> -const u8 crc7_syndrome_table[256] = {
> - 0x00, 0x09, 0x12, 0x1b, 0x24, 0x2d, 0x36, 0x3f,
> - 0x48, 0x41, 0x5a, 0x53, 0x6c, 0x65, 0x7e, 0x77,
> - 0x19, 0x10, 0x0b, 0x02, 0x3d, 0x34, 0x2f, 0x26,
> - 0x51, 0x58, 0x43, 0x4a, 0x75, 0x7c, 0x67, 0x6e,
> - 0x32, 0x3b, 0x20, 0x29, 0x16, 0x1f, 0x04, 0x0d,
> - 0x7a, 0x73, 0x68, 0x61, 0x5e, 0x57, 0x4c, 0x45,
> - 0x2b, 0x22, 0x39, 0x30, 0x0f, 0x06, 0x1d, 0x14,
> - 0x63, 0x6a, 0x71, 0x78, 0x47, 0x4e, 0x55, 0x5c,
> - 0x64, 0x6d, 0x76, 0x7f, 0x40, 0x49, 0x52, 0x5b,
> - 0x2c, 0x25, 0x3e, 0x37, 0x08, 0x01, 0x1a, 0x13,
> - 0x7d, 0x74, 0x6f, 0x66, 0x59, 0x50, 0x4b, 0x42,
> - 0x35, 0x3c, 0x27, 0x2e, 0x11, 0x18, 0x03, 0x0a,
> - 0x56, 0x5f, 0x44, 0x4d, 0x72, 0x7b, 0x60, 0x69,
> - 0x1e, 0x17, 0x0c, 0x05, 0x3a, 0x33, 0x28, 0x21,
> - 0x4f, 0x46, 0x5d, 0x54, 0x6b, 0x62, 0x79, 0x70,
> - 0x07, 0x0e, 0x15, 0x1c, 0x23, 0x2a, 0x31, 0x38,
> - 0x41, 0x48, 0x53, 0x5a, 0x65, 0x6c, 0x77, 0x7e,
> - 0x09, 0x00, 0x1b, 0x12, 0x2d, 0x24, 0x3f, 0x36,
> - 0x58, 0x51, 0x4a, 0x43, 0x7c, 0x75, 0x6e, 0x67,
> - 0x10, 0x19, 0x02, 0x0b, 0x34, 0x3d, 0x26, 0x2f,
> - 0x73, 0x7a, 0x61, 0x68, 0x57, 0x5e, 0x45, 0x4c,
> - 0x3b, 0x32, 0x29, 0x20, 0x1f, 0x16, 0x0d, 0x04,
> - 0x6a, 0x63, 0x78, 0x71, 0x4e, 0x47, 0x5c, 0x55,
> - 0x22, 0x2b, 0x30, 0x39, 0x06, 0x0f, 0x14, 0x1d,
> - 0x25, 0x2c, 0x37, 0x3e, 0x01, 0x08, 0x13, 0x1a,
> - 0x6d, 0x64, 0x7f, 0x76, 0x49, 0x40, 0x5b, 0x52,
> - 0x3c, 0x35, 0x2e, 0x27, 0x18, 0x11, 0x0a, 0x03,
> - 0x74, 0x7d, 0x66, 0x6f, 0x50, 0x59, 0x42, 0x4b,
> - 0x17, 0x1e, 0x05, 0x0c, 0x33, 0x3a, 0x21, 0x28,
> - 0x5f, 0x56, 0x4d, 0x44, 0x7b, 0x72, 0x69, 0x60,
> - 0x0e, 0x07, 0x1c, 0x15, 0x2a, 0x23, 0x38, 0x31,
> - 0x46, 0x4f, 0x54, 0x5d, 0x62, 0x6b, 0x70, 0x79
> +/*
> + * Table for CRC-7 (polynomial x^7 + x^3 + 1).
> + * This is a big-endian CRC (msbit is highest power of x),
> + * aligned so the msbit of the byte is the x^6 coefficient
> + * and the lsbit is not used.
> + */
> +const u8 crc7_be_syndrome_table[256] = {
> + 0x00, 0x12, 0x24, 0x36, 0x48, 0x5a, 0x6c, 0x7e,
> + 0x90, 0x82, 0xb4, 0xa6, 0xd8, 0xca, 0xfc, 0xee,
> + 0x32, 0x20, 0x16, 0x04, 0x7a, 0x68, 0x5e, 0x4c,
> + 0xa2, 0xb0, 0x86, 0x94, 0xea, 0xf8, 0xce, 0xdc,
> + 0x64, 0x76, 0x40, 0x52, 0x2c, 0x3e, 0x08, 0x1a,
> + 0xf4, 0xe6, 0xd0, 0xc2, 0xbc, 0xae, 0x98, 0x8a,
> + 0x56, 0x44, 0x72, 0x60, 0x1e, 0x0c, 0x3a, 0x28,
> + 0xc6, 0xd4, 0xe2, 0xf0, 0x8e, 0x9c, 0xaa, 0xb8,
> + 0xc8, 0xda, 0xec, 0xfe, 0x80, 0x92, 0xa4, 0xb6,
> + 0x58, 0x4a, 0x7c, 0x6e, 0x10, 0x02, 0x34, 0x26,
> + 0xfa, 0xe8, 0xde, 0xcc, 0xb2, 0xa0, 0x96, 0x84,
> + 0x6a, 0x78, 0x4e, 0x5c, 0x22, 0x30, 0x06, 0x14,
> + 0xac, 0xbe, 0x88, 0x9a, 0xe4, 0xf6, 0xc0, 0xd2,
> + 0x3c, 0x2e, 0x18, 0x0a, 0x74, 0x66, 0x50, 0x42,
> + 0x9e, 0x8c, 0xba, 0xa8, 0xd6, 0xc4, 0xf2, 0xe0,
> + 0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54, 0x62, 0x70,
> + 0x82, 0x90, 0xa6, 0xb4, 0xca, 0xd8, 0xee, 0xfc,
> + 0x12, 0x00, 0x36, 0x24, 0x5a, 0x48, 0x7e, 0x6c,
> + 0xb0, 0xa2, 0x94, 0x86, 0xf8, 0xea, 0xdc, 0xce,
> + 0x20, 0x32, 0x04, 0x16, 0x68, 0x7a, 0x4c, 0x5e,
> + 0xe6, 0xf4, 0xc2, 0xd0, 0xae, 0xbc, 0x8a, 0x98,
> + 0x76, 0x64, 0x52, 0x40, 0x3e, 0x2c, 0x1a, 0x08,
> + 0xd4, 0xc6, 0xf0, 0xe2, 0x9c, 0x8e, 0xb8, 0xaa,
> + 0x44, 0x56, 0x60, 0x72, 0x0c, 0x1e, 0x28, 0x3a,
> + 0x4a, 0x58, 0x6e, 0x7c, 0x02, 0x10, 0x26, 0x34,
> + 0xda, 0xc8, 0xfe, 0xec, 0x92, 0x80, 0xb6, 0xa4,
> + 0x78, 0x6a, 0x5c, 0x4e, 0x30, 0x22, 0x14, 0x06,
> + 0xe8, 0xfa, 0xcc, 0xde, 0xa0, 0xb2, 0x84, 0x96,
> + 0x2e, 0x3c, 0x0a, 0x18, 0x66, 0x74, 0x42, 0x50,
> + 0xbe, 0xac, 0x9a, 0x88, 0xf6, 0xe4, 0xd2, 0xc0,
> + 0x1c, 0x0e, 0x38, 0x2a, 0x54, 0x46, 0x70, 0x62,
> + 0x8c, 0x9e, 0xa8, 0xba, 0xc4, 0xd6, 0xe0, 0xf2
> };
> -EXPORT_SYMBOL(crc7_syndrome_table);
> +EXPORT_SYMBOL(crc7_be_syndrome_table);
>
> /**
> * crc7 - update the CRC7 for the data buffer
> @@ -55,14 +60,17 @@ EXPORT_SYMBOL(crc7_syndrome_table);
> * Context: any
> *
> * Returns the updated CRC7 value.
> + * The CRC7 is left-aligned in the byte (the lsbit is always 0), as that
> + * makes the computation easier, and all callers want it in that form.
> + *
> */
> -u8 crc7(u8 crc, const u8 *buffer, size_t len)
> +u8 crc7_be(u8 crc, const u8 *buffer, size_t len)
> {
> while (len--)
> - crc = crc7_byte(crc, *buffer++);
> + crc = crc7_be_byte(crc, *buffer++);
> return crc;
> }
> -EXPORT_SYMBOL(crc7);
> +EXPORT_SYMBOL(crc7_be);
>
> MODULE_DESCRIPTION("CRC7 calculations");
> MODULE_LICENSE("GPL");
> --
> 1.9.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun 2014-05-11 00:35:26, George Spelvin wrote:
> This eliminates a 1-bit left shift in every single caller,
> and also makes the inner loop of the CRC computation more efficient.
>
> And purged #include <linux/crc7.h> from files that don't use it at all.
Looks good, but should not function name change when functionality got
completely different?
Pavel
>
> Signed-off-by: George Spelvin <[email protected]>
> ---
> Since all of the affected drivers are officially orphaned, this is
> being sent to a hodge-podge of people who touched the files recently.
> Assuming the MMC people don't complain, is it okay to send this through
> the wireless tree?
>
> drivers/mmc/host/mmc_spi.c | 2 +-
> drivers/net/wireless/ti/wl1251/acx.c | 1 -
> drivers/net/wireless/ti/wl1251/cmd.c | 1 -
> drivers/net/wireless/ti/wl1251/spi.c | 3 +-
> drivers/net/wireless/ti/wlcore/spi.c | 3 +-
> include/linux/crc7.h | 2 +-
> lib/crc7.c | 74 ++++++++++++++++++++----------------
> 7 files changed, 45 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> index 0a87e56913..2c9e879c09 100644
> --- a/drivers/mmc/host/mmc_spi.c
> +++ b/drivers/mmc/host/mmc_spi.c
> @@ -472,7 +472,7 @@ mmc_spi_command_send(struct mmc_spi_host *host,
> *cp++ = (u8)(arg >> 16);
> *cp++ = (u8)(arg >> 8);
> *cp++ = (u8)arg;
> - *cp++ = (crc7(0, &data->status[1], 5) << 1) | 0x01;
> + *cp++ = crc7(0, &data->status[1], 5) | 0x01;
>
> /* Then, read up to 13 bytes (while writing all-ones):
> * - N(CR) (== 1..8) bytes of all-ones
> diff --git a/drivers/net/wireless/ti/wl1251/acx.c b/drivers/net/wireless/ti/wl1251/acx.c
> index 5a4ec56c83..5695628757 100644
> --- a/drivers/net/wireless/ti/wl1251/acx.c
> +++ b/drivers/net/wireless/ti/wl1251/acx.c
> @@ -2,7 +2,6 @@
>
> #include <linux/module.h>
> #include <linux/slab.h>
> -#include <linux/crc7.h>
>
> #include "wl1251.h"
> #include "reg.h"
> diff --git a/drivers/net/wireless/ti/wl1251/cmd.c b/drivers/net/wireless/ti/wl1251/cmd.c
> index bf1fa18b97..ede31f048e 100644
> --- a/drivers/net/wireless/ti/wl1251/cmd.c
> +++ b/drivers/net/wireless/ti/wl1251/cmd.c
> @@ -2,7 +2,6 @@
>
> #include <linux/module.h>
> #include <linux/slab.h>
> -#include <linux/crc7.h>
> #include <linux/etherdevice.h>
>
> #include "wl1251.h"
> diff --git a/drivers/net/wireless/ti/wl1251/spi.c b/drivers/net/wireless/ti/wl1251/spi.c
> index b06d36d993..47875fc6be 100644
> --- a/drivers/net/wireless/ti/wl1251/spi.c
> +++ b/drivers/net/wireless/ti/wl1251/spi.c
> @@ -122,8 +122,7 @@ static void wl1251_spi_wake(struct wl1251 *wl)
> crc[3] = cmd[6];
> crc[4] = cmd[5];
>
> - cmd[4] |= crc7(0, crc, WSPI_INIT_CMD_CRC_LEN) << 1;
> - cmd[4] |= WSPI_INIT_CMD_END;
> + cmd[4] = crc7(0, crc, WSPI_INIT_CMD_CRC_LEN) | WSPI_INIT_CMD_END;
>
> t.tx_buf = cmd;
> t.len = WSPI_INIT_CMD_LEN;
> diff --git a/drivers/net/wireless/ti/wlcore/spi.c b/drivers/net/wireless/ti/wlcore/spi.c
> index dbe826dd7c..8b3e38e453 100644
> --- a/drivers/net/wireless/ti/wlcore/spi.c
> +++ b/drivers/net/wireless/ti/wlcore/spi.c
> @@ -151,8 +151,7 @@ static void wl12xx_spi_init(struct device *child)
> crc[3] = cmd[6];
> crc[4] = cmd[5];
>
> - cmd[4] |= crc7(0, crc, WSPI_INIT_CMD_CRC_LEN) << 1;
> - cmd[4] |= WSPI_INIT_CMD_END;
> + cmd[4] = crc7(0, crc, WSPI_INIT_CMD_CRC_LEN) | WSPI_INIT_CMD_END;
>
> t.tx_buf = cmd;
> t.len = WSPI_INIT_CMD_LEN;
> diff --git a/include/linux/crc7.h b/include/linux/crc7.h
> index 1786e772d5..d2155dc953 100644
> --- a/include/linux/crc7.h
> +++ b/include/linux/crc7.h
> @@ -6,7 +6,7 @@ extern const u8 crc7_syndrome_table[256];
>
> static inline u8 crc7_byte(u8 crc, u8 data)
> {
> - return crc7_syndrome_table[(crc << 1) ^ data];
> + return crc7_syndrome_table[crc ^ data];
> }
>
> extern u8 crc7(u8 crc, const u8 *buffer, size_t len);
> diff --git a/lib/crc7.c b/lib/crc7.c
> index f1c3a144ce..4a73b26e7a 100644
> --- a/lib/crc7.c
> +++ b/lib/crc7.c
> @@ -10,40 +10,45 @@
> #include <linux/crc7.h>
>
>
> -/* Table for CRC-7 (polynomial x^7 + x^3 + 1) */
> +/*
> + * Table for CRC-7 (polynomial x^7 + x^3 + 1).
> + * This is a big-endian CRC (msbit is highest power of x),
> + * aligned so the msbit of the byte is the x^6 coefficient
> + * and the lsbit is not used.
> + */
> const u8 crc7_syndrome_table[256] = {
> - 0x00, 0x09, 0x12, 0x1b, 0x24, 0x2d, 0x36, 0x3f,
> - 0x48, 0x41, 0x5a, 0x53, 0x6c, 0x65, 0x7e, 0x77,
> - 0x19, 0x10, 0x0b, 0x02, 0x3d, 0x34, 0x2f, 0x26,
> - 0x51, 0x58, 0x43, 0x4a, 0x75, 0x7c, 0x67, 0x6e,
> - 0x32, 0x3b, 0x20, 0x29, 0x16, 0x1f, 0x04, 0x0d,
> - 0x7a, 0x73, 0x68, 0x61, 0x5e, 0x57, 0x4c, 0x45,
> - 0x2b, 0x22, 0x39, 0x30, 0x0f, 0x06, 0x1d, 0x14,
> - 0x63, 0x6a, 0x71, 0x78, 0x47, 0x4e, 0x55, 0x5c,
> - 0x64, 0x6d, 0x76, 0x7f, 0x40, 0x49, 0x52, 0x5b,
> - 0x2c, 0x25, 0x3e, 0x37, 0x08, 0x01, 0x1a, 0x13,
> - 0x7d, 0x74, 0x6f, 0x66, 0x59, 0x50, 0x4b, 0x42,
> - 0x35, 0x3c, 0x27, 0x2e, 0x11, 0x18, 0x03, 0x0a,
> - 0x56, 0x5f, 0x44, 0x4d, 0x72, 0x7b, 0x60, 0x69,
> - 0x1e, 0x17, 0x0c, 0x05, 0x3a, 0x33, 0x28, 0x21,
> - 0x4f, 0x46, 0x5d, 0x54, 0x6b, 0x62, 0x79, 0x70,
> - 0x07, 0x0e, 0x15, 0x1c, 0x23, 0x2a, 0x31, 0x38,
> - 0x41, 0x48, 0x53, 0x5a, 0x65, 0x6c, 0x77, 0x7e,
> - 0x09, 0x00, 0x1b, 0x12, 0x2d, 0x24, 0x3f, 0x36,
> - 0x58, 0x51, 0x4a, 0x43, 0x7c, 0x75, 0x6e, 0x67,
> - 0x10, 0x19, 0x02, 0x0b, 0x34, 0x3d, 0x26, 0x2f,
> - 0x73, 0x7a, 0x61, 0x68, 0x57, 0x5e, 0x45, 0x4c,
> - 0x3b, 0x32, 0x29, 0x20, 0x1f, 0x16, 0x0d, 0x04,
> - 0x6a, 0x63, 0x78, 0x71, 0x4e, 0x47, 0x5c, 0x55,
> - 0x22, 0x2b, 0x30, 0x39, 0x06, 0x0f, 0x14, 0x1d,
> - 0x25, 0x2c, 0x37, 0x3e, 0x01, 0x08, 0x13, 0x1a,
> - 0x6d, 0x64, 0x7f, 0x76, 0x49, 0x40, 0x5b, 0x52,
> - 0x3c, 0x35, 0x2e, 0x27, 0x18, 0x11, 0x0a, 0x03,
> - 0x74, 0x7d, 0x66, 0x6f, 0x50, 0x59, 0x42, 0x4b,
> - 0x17, 0x1e, 0x05, 0x0c, 0x33, 0x3a, 0x21, 0x28,
> - 0x5f, 0x56, 0x4d, 0x44, 0x7b, 0x72, 0x69, 0x60,
> - 0x0e, 0x07, 0x1c, 0x15, 0x2a, 0x23, 0x38, 0x31,
> - 0x46, 0x4f, 0x54, 0x5d, 0x62, 0x6b, 0x70, 0x79
> + 0x00, 0x12, 0x24, 0x36, 0x48, 0x5a, 0x6c, 0x7e,
> + 0x90, 0x82, 0xb4, 0xa6, 0xd8, 0xca, 0xfc, 0xee,
> + 0x32, 0x20, 0x16, 0x04, 0x7a, 0x68, 0x5e, 0x4c,
> + 0xa2, 0xb0, 0x86, 0x94, 0xea, 0xf8, 0xce, 0xdc,
> + 0x64, 0x76, 0x40, 0x52, 0x2c, 0x3e, 0x08, 0x1a,
> + 0xf4, 0xe6, 0xd0, 0xc2, 0xbc, 0xae, 0x98, 0x8a,
> + 0x56, 0x44, 0x72, 0x60, 0x1e, 0x0c, 0x3a, 0x28,
> + 0xc6, 0xd4, 0xe2, 0xf0, 0x8e, 0x9c, 0xaa, 0xb8,
> + 0xc8, 0xda, 0xec, 0xfe, 0x80, 0x92, 0xa4, 0xb6,
> + 0x58, 0x4a, 0x7c, 0x6e, 0x10, 0x02, 0x34, 0x26,
> + 0xfa, 0xe8, 0xde, 0xcc, 0xb2, 0xa0, 0x96, 0x84,
> + 0x6a, 0x78, 0x4e, 0x5c, 0x22, 0x30, 0x06, 0x14,
> + 0xac, 0xbe, 0x88, 0x9a, 0xe4, 0xf6, 0xc0, 0xd2,
> + 0x3c, 0x2e, 0x18, 0x0a, 0x74, 0x66, 0x50, 0x42,
> + 0x9e, 0x8c, 0xba, 0xa8, 0xd6, 0xc4, 0xf2, 0xe0,
> + 0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54, 0x62, 0x70,
> + 0x82, 0x90, 0xa6, 0xb4, 0xca, 0xd8, 0xee, 0xfc,
> + 0x12, 0x00, 0x36, 0x24, 0x5a, 0x48, 0x7e, 0x6c,
> + 0xb0, 0xa2, 0x94, 0x86, 0xf8, 0xea, 0xdc, 0xce,
> + 0x20, 0x32, 0x04, 0x16, 0x68, 0x7a, 0x4c, 0x5e,
> + 0xe6, 0xf4, 0xc2, 0xd0, 0xae, 0xbc, 0x8a, 0x98,
> + 0x76, 0x64, 0x52, 0x40, 0x3e, 0x2c, 0x1a, 0x08,
> + 0xd4, 0xc6, 0xf0, 0xe2, 0x9c, 0x8e, 0xb8, 0xaa,
> + 0x44, 0x56, 0x60, 0x72, 0x0c, 0x1e, 0x28, 0x3a,
> + 0x4a, 0x58, 0x6e, 0x7c, 0x02, 0x10, 0x26, 0x34,
> + 0xda, 0xc8, 0xfe, 0xec, 0x92, 0x80, 0xb6, 0xa4,
> + 0x78, 0x6a, 0x5c, 0x4e, 0x30, 0x22, 0x14, 0x06,
> + 0xe8, 0xfa, 0xcc, 0xde, 0xa0, 0xb2, 0x84, 0x96,
> + 0x2e, 0x3c, 0x0a, 0x18, 0x66, 0x74, 0x42, 0x50,
> + 0xbe, 0xac, 0x9a, 0x88, 0xf6, 0xe4, 0xd2, 0xc0,
> + 0x1c, 0x0e, 0x38, 0x2a, 0x54, 0x46, 0x70, 0x62,
> + 0x8c, 0x9e, 0xa8, 0xba, 0xc4, 0xd6, 0xe0, 0xf2
> };
> EXPORT_SYMBOL(crc7_syndrome_table);
>
> @@ -55,6 +60,9 @@ EXPORT_SYMBOL(crc7_syndrome_table);
> * Context: any
> *
> * Returns the updated CRC7 value.
> + * The CRC7 is left-aligned in the byte (the lsbit is always 0), as that
> + * makes the computation easier, and all callers want it in that form.
> + *
> */
> u8 crc7(u8 crc, const u8 *buffer, size_t len)
> {
> --
> 1.9.2
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 05/11/2014 03:02 AM, George Spelvin wrote:
> From 770aa22e6c9c92027e3e21797192ccabb3e7c70e Mon Sep 17 00:00:00 2001
> From: George Spelvin <[email protected]>
> Date: Sat, 10 May 2014 10:32:57 -0400
> Subject: [PATCH v2 1/3] lib/crc7: Shift crc7() output left 1 bit
>
> This eliminates a 1-bit left shift in every single caller,
> and makes the inner loop of the CRC computation more efficient.
>
> Renamed crc7 to crc7_be (big-endian) since the interface changed.
>
> Also purged #include <linux/crc7.h> from files that don't use it at all.
>
> Signed-off-by: George Spelvin <[email protected]>
If the whole point of this is to use it for MMC/SD cards, why not just
also subsume the OR 1 and call it crc7_mmc() or something like that.
(Which I'm all for doing... I don't know of any other crc7 users.)
-hpa
On 14 May 2014 14:23, George Spelvin <[email protected]> wrote:
> Ulf Hansson wrote:
>> Feel free to take this through the wireless tree as well. I assumes
>> it's that same patchset as the other spi related fixes?
>
> I'd like to answer, but I'm not sure which "other spi related fixes"
> you're referring to. If you mean patches 1 and 3 of this series, then
> obviously. If you mean something else then probably not.
>
> [PATCH 1/3] lib/crc7: Shift crc7() output left 1 bit
> [PATCH 3/3] drivers/net/wireless/ti/wl*/spi.c: Simplify CRC computation
>
Sorry for being unclear. It's those above I referred to.
Kind regards
Ulf Hansson
On Sun 2014-05-11 06:05:02, George Spelvin wrote:
> Very minor source and binary size reduction.
>
> Signed-off-by: George Spelvin <[email protected]>
Reviewed-by: Pavel Machek <[email protected]>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
H. Peter Anvin wrote:
> Seems to me to make the code easier to read, not harder. That was the
> whole point.
I thought it made it harder by moving the terminating 1 bit out of the
driver proper, where it has a symbolic name. Thus someone comparing
the driver to the spec has to read another source file to figure out
how the driver gets that bit set.
As it is, the straight-line code in the driver corresponds very simply
with the packet as illustrated in the data sheets.
I agree it's really really minor either way, but so is the saving
we're going for.
Since it's not *clearly* an improvement, I err on the side of not
messing with a driver I can't test.
On Wed, May 14, 2014 at 08:23:57AM -0400, George Spelvin wrote:
> Ulf Hansson wrote:
> > Feel free to take this through the wireless tree as well. I assumes
> > it's that same patchset as the other spi related fixes?
>
> I'd like to answer, but I'm not sure which "other spi related fixes"
> you're referring to. If you mean patches 1 and 3 of this series, then
> obviously. If you mean something else then probably not.
>
> [PATCH 1/3] lib/crc7: Shift crc7() output left 1 bit
> [PATCH 3/3] drivers/net/wireless/ti/wl*/spi.c: Simplify CRC computation
>
>
> As there seems to be no objection to sending the mmc change via the
> wireless tree, are they ready to be queued there?
>
> The additional reviews are:
> All 3: Reviewed-by: Pavel Machek <[email protected]>
> 2/3: Reviewed-by: Geert Uytterhoeven <[email protected]>
> 2/3: Acked-by: Ulf Hansson <[email protected]>
>
> I can resend everything with the additional signoff lines
> if that's easier for you.
I can take them, sure.
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Sun 2014-05-11 06:07:43, George Spelvin wrote:
> These devices require commands stored in buffers in an odd order,
> different from that in which the CRC is computed.
>
> Rather than make two copies of the commands in two different orders,
> form the commands in logical (CRC) order, append the CRC, then byte-swap
> in place to the desired order.
>
> The old code worked fine, I'm just scratching an "ugh, that's ugly"
> itch.
>
> Signed-off-by: George Spelvin <[email protected]>
Reviewed-by: Pavel Machek <[email protected]>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> Looks good, but should not function name change when functionality got
> completely different?
I couldn't think of one. Can you?
I suppose adding a _be (big-endian) suffix would be consistent.
Is that okay with you?
To do it properly, I have to rename all of:
crc7_syndrome_table[]
crc7_byte()
crc7()
even though the third is the only (in-tree) user of the first two.
Pavel Machek wrote;
> On Sun 2014-05-11 05:16:07, George Spelvin wrote:
>> To do it properly, I have to rename all of:
>>
>> crc7_syndrome_table[]
>> crc7_byte()
>> crc7()
>>
>> even though the third is the only (in-tree) user of the first two.
> If the first two are static, there's no problem.
They're not; they're exported from the header (even though, as
I mentioned, their only user is crc7()).
So my patch v2 1/3 renamed all three.
H. Peter Anvin wrote:
> If the whole point of this is to use it for MMC/SD cards, why not just
> also subsume the OR 1 and call it crc7_mmc() or something like that.
>
> (Which I'm all for doing... I don't know of any other crc7 users.)
You'll find all users in my patch series. 2/3 updates the MMC
card, while 3/3 hits drivers/net/wireless/ti/wl1251/spi.c and
drivers/net/wireless/ti/wlcore/spi.c (which I'm pretty sure aren't
MMC/SPI cards).
Now, it turns out that they *also* set the lsbit (calling it
WSPI_INIT_CMD_END). However, it's not possible to put that into the CRC
table (it would mess up all bytes but the last), so an explicitly coded
"| 1" is required at the end. Thic ends up being no saving at all to
execution path length, and only moves one instruction from three drivers
to shared code. While making it harder to read the drivers.
A microscopic space saving (if and only if you have more than one of these
drivers loaded) and no performance improvement didn't seem worth it to me.
On Sun 2014-05-11 06:02:11, George Spelvin wrote:
> >From 770aa22e6c9c92027e3e21797192ccabb3e7c70e Mon Sep 17 00:00:00 2001
> From: George Spelvin <[email protected]>
> Date: Sat, 10 May 2014 10:32:57 -0400
> Subject: [PATCH v2 1/3] lib/crc7: Shift crc7() output left 1 bit
>
> This eliminates a 1-bit left shift in every single caller,
> and makes the inner loop of the CRC computation more efficient.
>
> Renamed crc7 to crc7_be (big-endian) since the interface changed.
>
> Also purged #include <linux/crc7.h> from files that don't use it at all.
>
> Signed-off-by: George Spelvin <[email protected]>
Reviewed-by: Pavel Machek <[email protected]>
> v2: Functions renamed to reflect different results; Pavel Machek
> prompted me to think of something not too ugly.
Thanks!
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 11 May 2014 12:05, George Spelvin <[email protected]> wrote:
> Very minor source and binary size reduction.
>
> Signed-off-by: George Spelvin <[email protected]>
> ---
> I spotted this while making the previous crc7 change.
>
> This looks simple enough, but I don't actually have one of these devices to test.
> At least one other careful desk-check is solicited.
Acked-by: Ulf Hansson <[email protected]>
Feel free to take this through the wireless tree as well. I assumes
it's that same patchset as the other spi related fixes?
Kind regards
Ulf Hansson
>
> drivers/mmc/host/mmc_spi.c | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> index 338e2202ea..cc8d4a6099 100644
> --- a/drivers/mmc/host/mmc_spi.c
> +++ b/drivers/mmc/host/mmc_spi.c
> @@ -448,7 +448,6 @@ mmc_spi_command_send(struct mmc_spi_host *host,
> {
> struct scratch *data = host->data;
> u8 *cp = data->status;
> - u32 arg = cmd->arg;
> int status;
> struct spi_transfer *t;
>
> @@ -465,14 +464,12 @@ mmc_spi_command_send(struct mmc_spi_host *host,
> * We init the whole buffer to all-ones, which is what we need
> * to write while we're reading (later) response data.
> */
> - memset(cp++, 0xff, sizeof(data->status));
> + memset(cp, 0xff, sizeof(data->status));
>
> - *cp++ = 0x40 | cmd->opcode;
> - *cp++ = (u8)(arg >> 24);
> - *cp++ = (u8)(arg >> 16);
> - *cp++ = (u8)(arg >> 8);
> - *cp++ = (u8)arg;
> - *cp++ = crc7_be(0, &data->status[1], 5) | 0x01;
> + cp[1] = 0x40 | cmd->opcode;
> + put_unaligned_be32(cmd->arg, cp+2);
> + cp[6] = crc7_be(0, cp+1, 5) | 0x01;
> + cp += 7;
>
> /* Then, read up to 13 bytes (while writing all-ones):
> * - N(CR) (== 1..8) bytes of all-ones
> @@ -711,10 +708,7 @@ mmc_spi_writeblock(struct mmc_spi_host *host, struct spi_transfer *t,
> * so we have to cope with this situation and check the response
> * bit-by-bit. Arggh!!!
> */
> - pattern = scratch->status[0] << 24;
> - pattern |= scratch->status[1] << 16;
> - pattern |= scratch->status[2] << 8;
> - pattern |= scratch->status[3];
> + pattern = get_unaligned_be32(scratch->status);
>
> /* First 3 bit of pattern are undefined */
> pattern |= 0xE0000000;
> --
> 1.9.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun 2014-05-11 05:16:07, George Spelvin wrote:
> > Looks good, but should not function name change when functionality got
> > completely different?
>
> I couldn't think of one. Can you?
>
> I suppose adding a _be (big-endian) suffix would be consistent.
> Is that okay with you?
>
> To do it properly, I have to rename all of:
>
> crc7_syndrome_table[]
> crc7_byte()
> crc7()
>
> even though the third is the only (in-tree) user of the first two.
If the first two are static, there's no problem.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Ulf Hansson wrote:
> Feel free to take this through the wireless tree as well. I assumes
> it's that same patchset as the other spi related fixes?
I'd like to answer, but I'm not sure which "other spi related fixes"
you're referring to. If you mean patches 1 and 3 of this series, then
obviously. If you mean something else then probably not.
[PATCH 1/3] lib/crc7: Shift crc7() output left 1 bit
[PATCH 3/3] drivers/net/wireless/ti/wl*/spi.c: Simplify CRC computation
As there seems to be no objection to sending the mmc change via the
wireless tree, are they ready to be queued there?
The additional reviews are:
All 3: Reviewed-by: Pavel Machek <[email protected]>
2/3: Reviewed-by: Geert Uytterhoeven <[email protected]>
2/3: Acked-by: Ulf Hansson <[email protected]>
I can resend everything with the additional signoff lines
if that's easier for you.
Seems to me to make the code easier to read, not harder. That was the whole point.
On May 14, 2014 6:15:51 PM PDT, George Spelvin <[email protected]> wrote:
>H. Peter Anvin wrote:
>> If the whole point of this is to use it for MMC/SD cards, why not
>just
>> also subsume the OR 1 and call it crc7_mmc() or something like that.
>>
>> (Which I'm all for doing... I don't know of any other crc7 users.)
>
>You'll find all users in my patch series. 2/3 updates the MMC
>card, while 3/3 hits drivers/net/wireless/ti/wl1251/spi.c and
>drivers/net/wireless/ti/wlcore/spi.c (which I'm pretty sure aren't
>MMC/SPI cards).
>
>Now, it turns out that they *also* set the lsbit (calling it
>WSPI_INIT_CMD_END). However, it's not possible to put that into the
>CRC
>table (it would mess up all bytes but the last), so an explicitly coded
>"| 1" is required at the end. Thic ends up being no saving at all to
>execution path length, and only moves one instruction from three
>drivers
>to shared code. While making it harder to read the drivers.
>
>A microscopic space saving (if and only if you have more than one of
>these
>drivers loaded) and no performance improvement didn't seem worth it to
>me.
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
Very minor source and binary size reduction.
Signed-off-by: George Spelvin <[email protected]>
---
I spotted this while making the previous crc7 change.
This looks simple enough, but I don't actually have one of these devices to test.
At least one other careful desk-check is solicited.
drivers/mmc/host/mmc_spi.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 338e2202ea..cc8d4a6099 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -448,7 +448,6 @@ mmc_spi_command_send(struct mmc_spi_host *host,
{
struct scratch *data = host->data;
u8 *cp = data->status;
- u32 arg = cmd->arg;
int status;
struct spi_transfer *t;
@@ -465,14 +464,12 @@ mmc_spi_command_send(struct mmc_spi_host *host,
* We init the whole buffer to all-ones, which is what we need
* to write while we're reading (later) response data.
*/
- memset(cp++, 0xff, sizeof(data->status));
+ memset(cp, 0xff, sizeof(data->status));
- *cp++ = 0x40 | cmd->opcode;
- *cp++ = (u8)(arg >> 24);
- *cp++ = (u8)(arg >> 16);
- *cp++ = (u8)(arg >> 8);
- *cp++ = (u8)arg;
- *cp++ = crc7_be(0, &data->status[1], 5) | 0x01;
+ cp[1] = 0x40 | cmd->opcode;
+ put_unaligned_be32(cmd->arg, cp+2);
+ cp[6] = crc7_be(0, cp+1, 5) | 0x01;
+ cp += 7;
/* Then, read up to 13 bytes (while writing all-ones):
* - N(CR) (== 1..8) bytes of all-ones
@@ -711,10 +708,7 @@ mmc_spi_writeblock(struct mmc_spi_host *host, struct spi_transfer *t,
* so we have to cope with this situation and check the response
* bit-by-bit. Arggh!!!
*/
- pattern = scratch->status[0] << 24;
- pattern |= scratch->status[1] << 16;
- pattern |= scratch->status[2] << 8;
- pattern |= scratch->status[3];
+ pattern = get_unaligned_be32(scratch->status);
/* First 3 bit of pattern are undefined */
pattern |= 0xE0000000;
--
1.9.2