diff mbox

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

Message ID CANr=Z=Yo_wooTxVWLPycg65Q=DOxKr-BcdAYjXvnCmSAj6T9yQ@mail.gmail.com
State RFC
Headers show

Commit Message

Joe Hershberger May 25, 2012, 6:50 p.m. UTC 
Hi Michael,

On Thu, May 17, 2012 at 3:43 PM, Michael Walle <michael@walle.cc> wrote:
>
> 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@walle.cc> wrote:
>> > Signed-off-by: Michael Walle <michael@walle.cc>
>> > Cc: Joe Hershberger <joe.hershberger@gmail.com>
>> > ----
>> > 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

How about something like this instead...

-----------8<-----------8<-----------8<-----------8<-----------


----------->8----------->8----------->8----------->8-----------

And then in your board support...

-----------8<-----------8<-----------8<-----------8<-----------

diff --git a/board/buffalo/lsxl/lsxl.c b/board/buffalo/lsxl/lsxl.c
new file mode 100644
index 0000000..603d11a
--- /dev/null
+++ b/board/buffalo/lsxl/lsxl.c
+...
+
+static void rescue_mode(void)
+{
+	uchar enetaddr[6];
+	char enetvar[32];
+
+	printf("Entering rescue mode..\n");
+	if (!eth_getenv_enetaddr_by_index("eth", 0, enetaddr)) {
+		eth_random_enetaddr(enetaddr);
+		eth_enetaddr_var_by_index("eth", 0, enetvar);
+		if (eth_setenv_enetaddr(enetvar, enetaddr)) {
+			printf("Failed to set ethernet address\n");
+				set_led(LED_ALARM_BLINKING);
+			return;
+		}
+	}
+	setenv("bootsource", "rescue");
+}
+

----------->8----------->8----------->8----------->8-----------

That way you aren't adding a function that noone else uses and you
don't need a silly guard around it.

Cheers,
-Joe

Comments

Michael Walle May 25, 2012, 7:54 p.m. UTC | #1 
Am Freitag 25 Mai 2012, 20:50:09 schrieb Joe Hershberger:
[..snip..]
> That way you aren't adding a function that noone else uses and you
> don't need a silly guard around it.

Hi Joe,

thanks for the review. I think i'll drop the dynamic "ethernet variable name" 
support entirely. Two reasons:
 - it isn't really dynamic, eg. the function always evaluates the name to
   "ethaddr"
 - back then when i wrote the support for the board the name was "eth1addr"
   and mike suggested to use the by index function for the getter and a new
   function for the setter. But i guess its not worth the hassle ;)

btw, imho your solution introduces a discrepancy between the setter and 
getter. i hope it doesnt bother you if i don't adapt your solution. But thanks 
for the work.
Joe Hershberger May 25, 2012, 8:50 p.m. UTC | #2 
Hi Michael,

On Fri, May 25, 2012 at 2:54 PM, Michael Walle <michael@walle.cc> wrote:
> Am Freitag 25 Mai 2012, 20:50:09 schrieb Joe Hershberger:
> [..snip..]
>> That way you aren't adding a function that noone else uses and you
>> don't need a silly guard around it.
>
> Hi Joe,
>
> thanks for the review. I think i'll drop the dynamic "ethernet variable name"
> support entirely. Two reasons:
>  - it isn't really dynamic, eg. the function always evaluates the name to
>   "ethaddr"
>  - back then when i wrote the support for the board the name was "eth1addr"
>   and mike suggested to use the by index function for the getter and a new
>   function for the setter. But i guess its not worth the hassle ;)

OK.  Since it is specific to your board support anyway, that seems fine.

> btw, imho your solution introduces a discrepancy between the setter and
> getter.

It does, yes.  There is an inherent discrepancy, though, given that
one is used and the other is not.  I have no problem adding the
accessor if there are clients for it to justify its code space.

> i hope it doesnt bother you if i don't adapt your solution. But thanks
> for the work.

Not a problem at all.

-Joe
diff mbox

Patch

diff --git a/include/net.h b/include/net.h
index a092f29..5bc4b0d 100644
--- a/include/net.h
+++ b/include/net.h
@@ -600,6 +600,22 @@  static inline int is_broadcast_ether_addr(const u8 *addr)
 }

 /*
+ * Create a variable name for the ethernet address
+ *
+ * @param base_name Input string constant (typically "eth") prepended to name
+ * @param index Index to be added to the name if not 0
+ * @param ethaddr_var Output string variable name for MAC address
+ */
+static inline void eth_enetaddr_var_by_index(const char *base_name, int index,
+	char *ethaddr_var)
+{
+	if (index)
+		sprintf(ethaddr_var, "%s%daddr", base_name, index);
+	else
+		sprintf(ethaddr_var, "%saddr", base_name);
+}
+
+/*
  * is_valid_ether_addr - Determine if the given Ethernet address is valid
  * @addr: Pointer to a six-byte array containing the Ethernet address
  *
diff --git a/net/eth.c b/net/eth.c
index d9a6430..4b38bc4 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -58,7 +58,8 @@  int eth_getenv_enetaddr_by_index(const char
*base_name, int index,
 				 uchar *enetaddr)
 {
 	char enetvar[32];
-	sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
+
+	eth_enetaddr_var_by_index(base_name, index, enetvar);
 	return eth_getenv_enetaddr(enetvar, enetaddr);
 }