Re: [PATCH v3 21/24] C6X: general SoC support

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday 27 September 2011, Mark Salter wrote:
> +int soc_mac_addr(unsigned int index, u8 *addr)
> +{
> +       int i, have_cmdline_mac = 0, have_fuse_mac = 0;
> +
> +       for (i = 0; i < 6; i++) {
> +               if (cmdline_mac[i])
> +                       have_cmdline_mac = 1;
> +               if (c6x_fuse_mac[i])
> +                       have_fuse_mac = 1;
> +       }
> +
> +       /* cmdline overrides hardware fuse MAC */
> +       if (have_cmdline_mac)
> +               memcpy(addr, cmdline_mac, 6);
> +       else if (have_fuse_mac)
> +               memcpy(addr, c6x_fuse_mac, 6);
> +       else
> +               return 0;

IMHO it's more important to let the user override the MAC address through
the device tree than the command line, although if you allow both, the
command line should take precedence. If you think you need to keep the
command line option, please add a way to read it from the device tree
as a fallback before falling back to the fused mac address.

Also, a final fallback on random_mac_address() would be reasonable here.

Finally, how about passing a struct device pointer from the driver, rather
than the index? That would seem more logical and make it easier to get
to the device tree properties.

> +       /* adjust for specific EMAC device */
> +       addr[5] += index * c6x_num_cores;
> +       return 1;
> +}
> +EXPORT_SYMBOL(soc_mac_addr);

EXPORT_SYMBOL_GPL()

	Arnd