[U-Boot] [PATCH v5 4/5] net: add eth_setenv_enetaddr_by_index()

[Michael Walle]

Hi Joe,

Am Mittwoch 16 Mai 2012, 02:56:39 schrieb Joe Hershberger:
> Hi Michael,
> 
> On Fri, May 11, 2012 at 5:50 PM, Michael Walle <michael at walle.cc> wrote:
> > Signed-off-by: Michael Walle <michael at walle.cc>
> > Cc: Joe Hershberger <joe.hershberger at gmail.com>
> > ---
> >  include/net.h |   16 ++++++++++++++++
> >  net/eth.c     |   15 +++++++++++++++
> >  2 files changed, 31 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/net.h b/include/net.h
> > index f6aeba2..bdc3da6 100644
> > --- a/include/net.h
> > +++ b/include/net.h
> > @@ -104,7 +104,9 @@ extern struct eth_device *eth_get_dev_by_index(int
> > index); /* get dev @ index */ extern int eth_get_dev_index (void);      
> >     /* get the device index */ extern void eth_parse_enetaddr(const char
> > *addr, uchar *enetaddr); extern int eth_getenv_enetaddr(char *name,
> > uchar *enetaddr);
> > +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX
> >  extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr);
> > +#endif
> 
> Which other function did you intend?  You already guarded the only
> definition whose implementation is guarded.
That was a mistake. Will be fixed in the next version.

> >  /*
> >  * Get the hardware address for an ethernet interface .
> > @@ -118,6 +120,20 @@ extern int eth_setenv_enetaddr(char *name, const
> > uchar *enetaddr); extern int eth_getenv_enetaddr_by_index(const char
> > *base_name, int index, uchar *enetaddr);
> > 
> > +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX
> 
> Personally I don't like config options for something so tiny.  If you
> really must guard against including it, use the CONFIG_RANDOM_MACADDR
> instead.
Me, too. But Wolfgang doesn't accept patches which increases the code size for 
other boards.

I don't think it is a good idea to use CONFIG_RANDOM_MACADDR for such a 
generic function, which will (hopefully) be used by others too.

> I'd prefer to just let the linker exclude it when it's never
> referenced.
Correct, me if i'm wrong, but the linker can only discard unused sections. But 
there was some discussion on -ffunction-sections and -fdata-sections on the 
ML. So maybe theres some light at the end of the tunnel ;)

> 
> > +/*
> > + * Set the hardware address for an ethernet interface .
> > + * Args:
> > + *     base_name - base name for device (normally "eth")
> > + *     index - device index number (0 for first)
> > + *     enetaddr - returns 6 byte hardware address
> > + * Returns:
> > + *     0 on success, else 1.
> > + */
> > +extern int eth_setenv_enetaddr_by_index(const char *base_name, int
> > index, +                                       const uchar *enetaddr);
> > +#endif
> > +
> >  #ifdef CONFIG_RANDOM_MACADDR
> >  /*
> >  * The u-boot policy does not allow hardcoded ethernet addresses. Under
> > the diff --git a/net/eth.c b/net/eth.c
> > index afce863..d66e22a 100644
> > --- a/net/eth.c
> > +++ b/net/eth.c
> > @@ -67,6 +67,21 @@ int eth_getenv_enetaddr_by_index(const char
> > *base_name, int index, return eth_getenv_enetaddr(enetvar, enetaddr);
> >  }
> > 
> > +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX
> > +int eth_setenv_enetaddr_by_index(const char *base_name, int index,
> > +               const uchar *enetaddr)
> > +{
> > +       char enetvar[32];
> > +
> > +       if (index)
> > +               sprintf(enetvar, "%s%daddr", base_name, index);
> > +       else
> > +               sprintf(enetvar, "%saddr", base_name);
> > +
> > +       return eth_setenv_enetaddr(enetvar, enetaddr);
> > +}
> > +#endif
> 
> You should synchronize with Rob Herring who is also trying to add this
> eth_setenv_enetaddr_by_index() function.
> http://patchwork.ozlabs.org/patch/152590/
ah thanks for the hint.

> 
> Also since Rob's patch would use this function all the time, the
> CONFIG_SETENV_ENETADDR_BY_INDEX would be removed, right?
right, but see above.

-- 
Michael