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? > > + 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. Best regards, Codrin _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
