[U-Boot] [PATCH 01/11 v2] drivers/net/vsc9953: Cleanup patch
Codrin Constantin Ciubotariu
codrin.ciubotariu at freescale.com
Thu Jun 25 18:35:40 CEST 2015
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
More information about the U-Boot
mailing list