[U-Boot] [PATCH 01/11 v2] drivers/net/vsc9953: Cleanup patch

[Joe Hershberger]

Hi Codrin,

On Thu, Jun 25, 2015 at 11:35 AM, Codrin Constantin Ciubotariu
<codrin.ciubotariu at 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