[PATCH v3 1/3] net: cortina_ni: Add eth support for Cortina Access CAxxxx SoCs

[Alex Nemirovsky]

Abbie,

Please have a look at Tom’s feedback to address his concerns.  

> On Sep 29, 2020, at 10:55 AM, Tom Rini <trini at konsulko.com> wrote:
> 
> On Wed, Sep 23, 2020 at 05:59:12PM -0700, Alex Nemirovsky wrote:
> 
>> From: Abbie Chang <abbie.chang at 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.tseng at cortina-access.com>
>> Signed-off-by: Alex Nemirovsky <alex.nemirovsky at cortina-access.com>
>> Signed-off-by: Abbie Chang <abbie.chang at cortina-access.com>
>> 
>> CC: Joe Hershberger <joe.hershberger at ni.com>
>> CC: Abbie Chang <abbie.chang at Cortina-Access.com>
>> CC: Tom Rini <trini at 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