Hi Andrey,
> I did see that patch go through, however since the title of it is
> "arm: imx6: ocotp: Added support for the i.MX6UL" I didn't look into
> it any further.
In the light of the Vybrid also supports two MAC addresses your patch title
i.MX: ocotp: Add provisions for storing multiple MAC addresses
is better, since it is more generic.
> > Your approach uses an extra device attribute 'mac_idx' to select the meaning
> > of the device attribute 'mac_addr': Whether it points to the MAC0 or MAC1.
> >
> > I find it less error prone and confusing to use two seperate device attributes
> > 'mac_addr' and 'mac_addr1' for MAC0 and MAC1. So you don't have to check the
> > value of 'mac_idx before reading or writing the MAC addresses.
>
> Can't say that I necessarily agree with you assessment. There's no
> need to check 'mac_idx' if it is written every time as a part of MAC
> address assignment operation. The reason I chose the approach that I
> did was mainly because it allowed to both have backwards compatibility
> with the old naming scheme and avoid having variable naming
> inconsistency (as you see in your example where one variable has a
> numerical suffix and the other doesn't) which I was concerned would be
> make scripting it more clunky than necessary.
hmm, I find it more convenient use
bootloader$ ocotp0.mac_addr=22:33:44:55:66:77
bootloader$ ocotp0.mac_addr1=22:33:44:AA:AA:AA
than
bootloader$ ocotp0.mac_idx=0
bootloader$ ocotp0.mac_addr=22:33:44:55:66:77
bootloader$ ocotp0.mac_idx=1
bootloader$ ocotp0.mac_addr=22:33:44:AA:AA:AA
The same goes for reading the MAC addresses. The first one is more obvious,
because the meaning of the variable "mac_addr" does not depend on another
variable "mac_idx".
Maybe we can settle on a different approach to avoid the inconsistent variable
names "mac_addr" and "mac_addr1":
if (IS_ENABLED(CONFIG_NET)) {
if (!data->scnd_mac_addr) {
/* one MAC */
dev_add_param_mac(&(priv->dev), "mac_addr", imx_ocotp_set_mac0,
imx_ocotp_get_mac0, priv->ethaddr[0], priv);
} else {
/* two MACs: Vybrid and UltraLite */
dev_add_param_mac(&(priv->dev), "mac_addr0", imx_ocotp_set_mac0,
imx_ocotp_get_mac0, priv->ethaddr[0], priv);
dev_add_param_mac(&(priv->dev), "mac_addr1", imx_ocotp_set_mac1,
imx_ocotp_get_mac1, priv->ethaddr[1], priv);
}
}
All existing boards still uses "mac_addr" for there single MAC address and new
boards Vybrid and UltraLite use "mac_addr0" and "mac_addr1". In the wild there
will be two different sets of - for example - factory MAC burning scripts
anyway. One to handle a single MAC address and another to handle two MAC
addresses.
What do you think?
Mit freundlichen Grüßen / Kind regards,
Stefan Lengfeld
On Tue, Dec 06, 2016 at 06:48:16AM -0800, Andrey Smirnov wrote:
> Hi Stefan,
>
> On Mon, Dec 5, 2016 at 7:14 AM, Stefan Lengfeld <s.lengfeld@xxxxxxxxx> wrote:
> > Hi Andrey,
> >
> > a similiar patch was posted some days ago to support two MAC addresses for the
> > i.MX6 UltraLite. See
> >
> > http://lists.infradead.org/pipermail/barebox/2016-November/028628.html
> >
>
> I did see that patch go through, however since the title of it is
> "arm: imx6: ocotp: Added support for the i.MX6UL" I didn't look into
> it any further.
>
>
> > Your approach uses an extra device attribute 'mac_idx' to select the meaning
> > of the device attribute 'mac_addr': Whether it points to the MAC0 or MAC1.
> >
> > I find it less error prone and confusing to use two seperate device attributes
> > 'mac_addr' and 'mac_addr1' for MAC0 and MAC1. So you don't have to check the
> > value of 'mac_idx before reading or writing the MAC addresses.
>
> Can't say that I necessarily agree with you assessment. There's no
> need to check 'mac_idx' if it is written every time as a part of MAC
> address assignment operation. The reason I chose the approach that I
> did was mainly because it allowed to both have backwards compatibility
> with the old naming scheme and avoid having variable naming
> inconsistency (as you see in your example where one variable has a
> numerical suffix and the other doesn't) which I was concerned would be
> make scripting it more clunky than necessary.
>
> Anyway, I don't think I am impartial enough to make a good judge of
> merits of both approaches, so I'll let Sascha decide which way he
> wants to go.
>
> Thanks,
> Andrey Smirnov
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
