[prev in list] [next in list] [prev in thread] [next in thread] 

List:       u-boot
Subject:    Re: [U-Boot] [PATCH 08/11 v2] drivers/net/vsc9953: Add VLAN commands for VSC9953
From:       Joe Hershberger <joe.hershberger () gmail ! com>
Date:       2015-06-30 22:52:31
Message-ID: CANr=Z=ZGzEAS=mDBJGf_6OntRM9hUkPwUHtOH7NC=ZsUbt=mMw () mail ! gmail ! com
[Download RAW message or body]

Hi Codrin,

On Tue, Jun 30, 2015 at 9:02 AM, Codrin Constantin Ciubotariu
<codrin.ciubotariu@freescale.com> wrote:
> Hi Joe,
> 
> > -----Original Message-----
> > From: Joe Hershberger [mailto:joe.hershberger@gmail.com]
> > Sent: Friday, June 26, 2015 1:41 AM
> > To: Ciubotariu Codrin Constantin-B43658
> > Cc: u-boot; Joe Hershberger; Sun York-R58495
> > Subject: Re: [U-Boot] [PATCH 08/11 v2] drivers/net/vsc9953: Add VLAN commands
> > for VSC9953
> > 
> > > @@ -270,6 +270,31 @@ static void vsc9953_port_vlan_pvid_set(int port_no, int
> > pvid)
> > > field_set(pvid,
> > CONFIG_VSC9953_PORT_VLAN_CFG_VID_MASK));
> > > }
> > > 
> > > +#ifdef CONFIG_VSC9953_CMD
> > 
> > Why does this need to be defined outside of the #ifdef already at the
> > bottom of the file?
> 
> I added it here so that vsc9953_port_vlan_pvid_get() could be next to its pair, \
> vsc9953_port_vlan_pvid_set(). If you think that it should be at the bottom of the \
> file I can move it.

OK, I think that makes sense the way you had it. Also, it wouldn't
make sense to move out of drivers/net/vsc9953.c, so leave it here.

> > > +       /* Administrative down */
> > > +       if ((!vsc9953_l2sw.port[port_nr].enabled)) {
> > 
> > Why do you have double "((" and "))"?
> 
> By mistake, I will remove one pair.
> 
> > > +#ifdef CONFIG_VSC9953_CMD
> > 
> > Why does this need to be defined outside of the #ifdef already at the
> > bottom of the file?
> 
> I did this so that similar get/set() functions to be close to one another. Also, \
> the functions guarded by CONFIG_VSC9953_CMD are used only when the ethsw commands \
> are enabled (CONFIG_VSC9953_CMD defined). If a user decides to use the driver for \
> VSC9953, with the switch working in unmanaged state and he doesn't need the \
> commands to configure the switch, he could compile u-boot with CONFIG_VSC9953_CMD \
> undefined. In this case, some warnings will appear at compile time suggesting that \
> functions like vsc9953_port_vlan_egr_untag_get() are defined but not used.

Sounds good. You can leave this here.

Thanks,
-Joe
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


[prev in list] [next in list] [prev in thread] [next in thread]