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