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

[Alex Nemirovsky]

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 <Alex.Nemirovsky at cortina-access.com> wrote:
> 
> 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