Hi Codrin,
On Thu, Jun 25, 2015 at 11:35 AM, Codrin Constantin Ciubotariu codrin.ciubotariu@freescale.com wrote:
Hi Joe,
/* alloc eth device */ dev = (struct eth_device *)calloc(1, sizeof(struct eth_device)); if (!dev)
return 1;
return -1;Is it reasonable to use values from asm/errno.h here and elsewhere in the driver? This seems like it should return -ENODEV instead of -EPERM.
Yes, I should use values from errno.h . -ENOMEM seems more appropriate to me. What do you think?
Yes, sorry. I didn't look at the line above! :/
enabled = !!(vsc9953_l2sw.port[port_no].enabled &val & CONFIG_VSC9953_PORT_ENA);This is incorrect... Should be:
enabled = vsc9953_l2sw.port[port_no].enabled &&(val & CONFIG_VSC9953_PORT_ENA);Ok.
diff --git a/include/vsc9953.h b/include/vsc9953.h index 3d11b87..920402f 100644 --- a/include/vsc9953.h +++ b/include/vsc9953.h @@ -33,29 +33,60 @@ #define T1040_SWITCH_GMII_DEV_OFFSET 0x010000 #define VSC9953_PHY_REGS_OFFST 0x0000AC
+/* Macros for vsc9953_chip_regs.soft_rst register */ #define CONFIG_VSC9953_SOFT_SWC_RST_ENA 0x00000001
All of there that are register constants should not have "CONFIG_" prepended to them. That should only be for constants that configure something, eventually only from Kconfig. Please add another patch before this one that removes that.
Ok, I will add another patch before this one.
+/* Macros for vsc9953_sys_pause_cfg.pause_cfg register */ #define CONFIG_VSC9953_PAUSE_CFG 0x001ffffe
+/* Macros for vsc9953_sys_pause_cfg.pause_cfg register */ +#define CONFIG_VSC9953_PAUSE_CFG 0x001ffffe
This adds a duplicate of the define above it.
I will remove one of them.
+/* Macros for vsc9953_vcap_core_cfg.vcap_mv_cfg register */ #define CONFIG_VSC9953_VCAP_MV_CFG 0x0000ffff #define CONFIG_VSC9953_VCAP_UPDATE_CTRL 0x01000004
May as well get rid of the tabs here after the defines.
Ok.
Thank you for your review. I will make v3.
Sure thing. I'll try to get through the rest of them today.
Cheers, -Joe