[PATCH 02/16] i.MX: ocotp: Add provisions for storing multiple MAC addresses

[Stefan Lengfeld]

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 at phytec.de> 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