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 <tedc...@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!
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to