Abbie, Please have a look at Tom’s feedback to address his concerns.
> On Sep 29, 2020, at 10:55 AM, Tom Rini <tr...@konsulko.com> wrote: > > On Wed, Sep 23, 2020 at 05:59:12PM -0700, Alex Nemirovsky wrote: > >> From: Abbie Chang <abbie.ch...@cortina-access.com> >> >> Add Cortina Access Ethernet device driver for CAxxxx SoCs. >> This driver supports both legacy and DM_ETH network models. >> >> Signed-off-by: Aaron Tseng <aaron.ts...@cortina-access.com> >> Signed-off-by: Alex Nemirovsky <alex.nemirov...@cortina-access.com> >> Signed-off-by: Abbie Chang <abbie.ch...@cortina-access.com> >> >> CC: Joe Hershberger <joe.hershber...@ni.com> >> CC: Abbie Chang <abbie.ch...@cortina-access.com> >> CC: Tom Rini <tr...@konsulko.com> > [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