Moved my comment from prior response so that it’s more likely to be seen:
Thanks for your patience on this one. I know we have been successful with most of the other code that has been submitted upstream. This one seems to be a challenge. Could you give us your honest constructive criticism on this code and I will be happy to pass it up to management for consideration. Thanks Tom. > On Sep 29, 2020, at 11:12 AM, Alex Nemirovsky > <[email protected]> wrote: > > Abbie, > > Please have a look at Tom’s feedback to address his concerns. > >>> On Sep 29, 2020, at 10:55 AM, Tom Rini <[email protected]> wrote: >>> >>> On Wed, Sep 23, 2020 at 05:59:12PM -0700, Alex Nemirovsky wrote: >>> >>> From: Abbie Chang <[email protected]> >>> >>> Add Cortina Access Ethernet device driver for CAxxxx SoCs. >>> This driver supports both legacy and DM_ETH network models. >>> >>> Signed-off-by: Aaron Tseng <[email protected]> >>> Signed-off-by: Alex Nemirovsky <[email protected]> >>> Signed-off-by: Abbie Chang <[email protected]> >>> >>> CC: Joe Hershberger <[email protected]> >>> CC: Abbie Chang <[email protected]> >>> CC: Tom Rini <[email protected]> >> [snip] >> >> Again, please give the whole driver a read and compare it with other >> network drivers that've been most recently added. I question things >> like: >>> +static u32 *rdwrptr_adv_one(u32 *x, unsigned long base, unsigned long max) >>> +{ >>> + if (x + 1 >= (u32 *)max) >>> + return (u32 *)base; >>> + else >>> + return (x + 1); >>> +} >> [snip] >>> +static u32 REG_TO_U32(void *reg) >>> +{ >>> + return *(u32 *)reg; >>> +} >> [snip] >>> +int ca_miiphy_read(const char *devname, >>> + unsigned char addr, >>> + unsigned char reg, >>> + unsigned short *value) >>> +{ >>> + return ca_mdio_read(addr, reg, value); >>> +} >>> + >>> +int ca_miiphy_write(const char *devname, >>> + unsigned char addr, >>> + unsigned char reg, >>> + unsigned short value) >>> +{ >>> + return ca_mdio_write(addr, reg, value); >>> +} >> >> And lots of other simple looking wrappers. >> >> [snip] >>> +static void ca_ni_setup_mac_addr(void) >>> +{ >>> + unsigned char mac[6]; >>> + >>> + struct NI_HV_GLB_MAC_ADDR_CFG0_t mac_addr_cfg0; >>> + struct NI_HV_GLB_MAC_ADDR_CFG1_t mac_addr_cfg1; >>> + struct NI_HV_PT_PORT_STATIC_CFG_t port_static_cfg; >>> + struct NI_HV_XRAM_CPUXRAM_CFG_t cpuxram_cfg; >>> + struct cortina_ni_priv *priv = dev_get_priv(curr_dev); >> >> While checkpatch didn't complain, can you please fix the driver to use >> consistent spacing? > Abbie, > Please look at the spacing. >> >> I feel like I asked before about Linux upstreaming of these drivers and >> the answer was that it was planned for later, is that right? > IIRC you asked for the Linux drivers and I responded that there are no Linux > drivers upstream and that management has not asked for them to be upstreamed. > > > > Thanks for your patience on this one. I know we have been successful with > most of the other code that has been submitted upstream. This one seems to > be a challenge. > Could you give us your honest constructive criticism on this code and I will > be happy to pass it up to management for consideration. > > Thanks Tom. >> >> -- >> Tom
