On Wednesday, November 25, 2015 at 06:30:54 AM, Ted Chen wrote:
From: Ted Chen <tedchen at realtek.com>
This patch adds driver support for the Realtek RTL8152B/RTL8153 USB network adapters.
Signed-off-by: Ted Chen <tedchen at realtek.com> [swarren, fixed a few compiler warnings] [swarren, with permission, converted license header to SPDX] [swarren, removed printf() spew during probe()] Signed-off-by: Stephen Warren <swarren at nvidia.com>
Changes for v2: Modified by Marek's comments.
- Remove pattern informations.
- Don't allocate & free when read/write register.
- relpace udelay to mdelay.
- pull firmware into global variable.
- code review.
The changelog should go into the diffstat part, otherwise it will be picked and become part of the commit message. We don't want that.
I only have nitpicks below :)
Signed-off-by: Ted Chen tedchen@realtek.com
drivers/usb/eth/Makefile | 1 + drivers/usb/eth/r8152.c | 3099 +++++++++++++++++++++++++++++++++++++++++++ drivers/usb/eth/usb_ether.c | 8 + include/usb_ether.h | 6 + 4 files changed, 3114 insertions(+) create mode 100644 drivers/usb/eth/r8152.c
The changelog should go here.
diff --git a/drivers/usb/eth/Makefile b/drivers/usb/eth/Makefile index c92d2b0..74f5f87 100644 --- a/drivers/usb/eth/Makefile +++ b/drivers/usb/eth/Makefile
[...]
+#define DRIVER_VERSION "v1.0 (2015/11/24)"
I don't think this is really relevant information.
+#define R8152_PHY_ID 32
[...]
+/* OCP_EEE_CONFIG3 */ +#define fast_snr_mask 0xff80 +#define fast_snr(x) (min(x, 0x1ff) << 7) /* bit 7 ~ 15 */
Please add parenthesis around the x -- min((x), ...
+#define RG_LFS_SEL 0x0060 /* bit 6 ~ 5 */ +#define MSK_PH 0x0006 /* bit 0 ~ 3 */
[...]
+#define msleep(a) mdelay(a)
Please just drop this.
+static u8 r8152b_pla_patch_a[] = {
- 0x08, 0xe0, 0x40, 0xe0, 0x78, 0xe0, 0x85, 0xe0,
Please add space after comma to make it consistent. Fix globally.
- 0x5d, 0xe1, 0xa1, 0xe1, 0xa3, 0xe1, 0xab, 0xe1,
[...]
+static u16 r8152b_ram_code1[] = {
- 0x9700, 0x7fe0, 0x4c00, 0x4007, 0x4400, 0x4800, 0x7c1f, 0x4c00,
Please drop the tab and add space. Fix globally.
- 0x5310, 0x6000, 0x7c07, 0x6800, 0x673e, 0x0000, 0x0000, 0x571f,
[...]
+static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
void *data, u16 type)+{
- u16 limit = 64;
- int ret = 0;
- /* both size and indix must be 4 bytes align */
- if ((size & 3) || !size || (index & 3) || !data)
return -EPERM;- if ((u32)index + (u32)size > 0xffff)
Are the casts here needed ?
return -EPERM;- while (size) {
if (size > limit) {ret = get_registers(tp, index, type, limit, data);if (ret < 0)break;index += limit;data += limit;size -= limit;} else {ret = get_registers(tp, index, type, size, data);if (ret < 0)break;index += size;data += size;size = 0;break;}- }
- return ret;
+}
+static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
u16 size, void *data, u16 type)+{
- int ret;
- u16 byteen_start, byteen_end, byen;
- u16 limit = 512;
- /* both size and indix must be 4 bytes align */
- if ((size & 3) || !size || (index & 3) || !data)
return -EPERM;- if ((u32)index + (u32)size > 0xffff)
return -EPERM;- byteen_start = byteen & BYTE_EN_START_MASK;
- byteen_end = byteen & BYTE_EN_END_MASK;
- byen = byteen_start | (byteen_start << 4);
- ret = set_registers(tp, index, type | byen, 4, data);
- if (ret < 0)
goto error1;- index += 4;
- data += 4;
- size -= 4;
- if (size) {
size -= 4;while (size) {if (size > limit) {ret = set_registers(tp, index,type | BYTE_EN_DWORD,limit, data);if (ret < 0)goto error1;index += limit;data += limit;size -= limit;
Cannot the branches of this code be rewritten in a more compact way ? It seems you're only decrementing two variables by a different factor, no?
} else {ret = set_registers(tp, index,type | BYTE_EN_DWORD,size, data);if (ret < 0)goto error1;index += size;data += size;size = 0;break;}}
[...]
+static u8 ocp_read_byte(struct r8152 *tp, u16 type, u16 index) +{
- u32 data;
- __le32 tmp;
- u8 shift = index & 3;
- index &= ~3;
- generic_ocp_read(tp, index, sizeof(tmp), &tmp, type);
- data = __le32_to_cpu(tmp);
- data >>= (shift * 8);
- data &= 0xff;
- return (u8)data;
The cast is not needed, you might want to check this al around the place.
+}
[...]
+static void r8153_firmware(struct r8152 *tp) +{
- int i;
- if (tp->version == RTL_VER_03) {
r8153_clear_bp(tp);r8153_pre_ram_code(tp, 0x7000);for (i = 0; i < ARRAY_SIZE(r8153_ram_code_a); i = i+2)ocp_write_word(tp, MCU_TYPE_PLA,r8153_ram_code_a[i],r8153_ram_code_a[i+1]);r8153_post_ram_code(tp);- } else if (tp->version == RTL_VER_04) {
r8153_pre_ram_code(tp, 0x7001);- for (i = 0; i < ARRAY_SIZE(r8153_ram_code_bc); i = i+2)
ocp_write_word(tp, MCU_TYPE_PLA,r8153_ram_code_bc[i],r8153_ram_code_bc[i+1]);r8153_post_ram_code(tp);r8153_wdt1_end(tp);r8153_clear_bp(tp);ocp_write_word(tp, MCU_TYPE_USB, USB_BP_EN, 0x0000);generic_ocp_write(tp, 0xf800, 0xff,sizeof(r8153_usb_patch_b),r8153_usb_patch_b, MCU_TYPE_USB);for (i = 0; i < ARRAY_SIZE(r8153_usb_patch_b_bp); i = i+2)
i += 2 is simpler, please fix globally
ocp_write_word(tp, MCU_TYPE_USB,r8153_usb_patch_b_bp[i],r8153_usb_patch_b_bp[i+1]);if (!(ocp_read_word(tp, MCU_TYPE_PLA, 0xd38e) & BIT(0))) {ocp_write_word(tp, MCU_TYPE_PLA, 0xd38c, 0x0082);ocp_write_word(tp, MCU_TYPE_PLA, 0xd38e, 0x0082);}ocp_write_word(tp, MCU_TYPE_PLA, PLA_BP_EN, 0x0000);generic_ocp_write(tp, 0xf800, 0xff,sizeof(r8153_pla_patch_b),r8153_pla_patch_b, MCU_TYPE_PLA);for (i = 0; i < ARRAY_SIZE(r8153_pla_patch_b_bp); i = i+2)ocp_write_word(tp, MCU_TYPE_PLA,r8153_pla_patch_b_bp[i],r8153_pla_patch_b_bp[i+1]);ocp_write_word(tp, MCU_TYPE_PLA, 0xd388, 0x08ca);
[...]
Otherwise looks OK, thanks!