Re: [PATCH] of_net: Implement of_get_nvmem_mac_address helper

[Florian Fainelli]

On 03/23/2018 12:20 PM, Mike Looijmans wrote:
> On 23-3-2018 16:11, Andrew Lunn wrote:
>> On Fri, Mar 23, 2018 at 03:24:34PM +0100, Mike Looijmans wrote:
>>> It's common practice to store MAC addresses for network interfaces into
>>> nvmem devices. However the code to actually do this in the kernel lacks,
>>> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
>>> address from an nvmem cell provider.
>>>
>>> This is particulary useful on devices where the ethernet interface
>>> cannot
>>> be configured by the bootloader, for example because it's in an FPGA.
>>>
>>> Tested by adapting the cadence macb driver to call this instead of
>>> of_get_mac_address().
>>
>> Hi Mike
>>
>> Please can you document the device tree binding. I assume you are
>> adding a nvmen-cells and nvmem-cell-names to the Ethernet node in
>> device tree.
> 
> Indeed. I'll add my settings as an example. Where should I put this
> documentation, in the commit comment or somewhere in
> Documents/devicetree/bindings?
> 
>>> +/**
>>> + * Search the device tree for a MAC address, by calling
>>> of_get_mac_address
>>> + * and if that doesn't provide an address, fetch it from an nvmem
>>> provider
>>> + * using the name 'mac-address'.
>>> + * On success, copies the new address is into memory pointed to by
>>> addr and
>>> + * returns 0. Returns a negative error code otherwise.
>>> + * @dev:ÂÂÂ Pointer to the device containing the device_node
>>> + * @addr:ÂÂÂ Pointer to receive the MAC address using ether_addr_copy()
>>> + */
>>> +int of_get_nvmem_mac_address(struct device *dev, char *addr)
>>> +{
>>> +ÂÂÂ const char *mac;
>>> +ÂÂÂ struct nvmem_cell *cell;
>>> +ÂÂÂ size_t len;
>>> +ÂÂÂ int ret;
>>> +
>>> +ÂÂÂ mac = of_get_mac_address(dev->of_node);
>>> +ÂÂÂ if (mac) {
>>> +ÂÂÂÂÂÂÂ ether_addr_copy(addr, mac);
>>> +ÂÂÂÂÂÂÂ return 0;
>>> +ÂÂÂ }
>>
>> Is there a need to add a new API? Could of_get_mac_address() be
>> extended to look in NVMEM? The MAC driver does not care. It is saying,
>> using OF get me a MAC address. One API seems sufficient, and would
>> mean you don't need to change the MAC drivers.
> 
> It's what I intended to do, but there were two problems with that:
> - of_get_mac_address() returns a pointer to constant data in memory, but
> the nvmem functions return an allocated memory object that must be freed
> after use. This changes the way the call is to be made.

Yeah...

> - The nvmem functions need the "struct device" pointer as well, while
> of_get_mac_address() only gets passed the DT node.

Bummer, you can't assume there is always a struct device associated with
a struct device_node. Also, bigger question is, how can we make this
work, for e.g: ACPI systems and therefore use an abstract fw_node handle?

> 
> One approach would be to deprecate the of_get_mac_address() interface
> and migrate existing drivers to the of_get_nvmem_mac_address() interface.

Humm maybe, but clearly making of_get_mac_address() look for a nvmem is
less error prone and does not require people to opt-in for the new
helper, that seems beneficial to me.
-- 
Florian