2021-04-12 17:48:20

by Michael Walle

[permalink] [raw]
Subject: [PATCH net-next v4 0/2] of: net: support non-platform devices in of_get_mac_address()

of_get_mac_address() is commonly used to fetch the MAC address
from the device tree. It also supports reading it from a NVMEM
provider. But the latter is only possible for platform devices,
because only platform devices are searched for a matching device
node.

Add a second method to fetch the NVMEM cell by a device tree node
instead of a "struct device".

Moreover, the NVMEM subsystem will return dynamically allocated
data which has to be freed after use. Currently, this is handled
by allocating a device resource manged buffer to store the MAC
address. of_get_mac_address() then returns a pointer to this
buffer. Without a device, this trick is not possible anymore.
Thus, change the of_get_mac_address() API to have the caller
supply a buffer.

It was considered to use the network device to attach the buffer
to, but then the order matters and netdev_register() has to be
called before of_get_mac_address(). No driver does it this way.

changes since v3:
- use memcpy() instead of ether_addr_copy() where appropriate.
Sometimes the destination is on the stack, thus the 2 byte
alignment requrement is not met.
- fix "return PTR_ERR(mac_addr)" as found by Dan Carpenter
- changed subject of patch 2/2, as suggested by Florian Fainelli

changes since v2:
- fixed of_get_mac_addr_nvmem() signature, which was accidentially
fixed in patch 2/2 again

changes since v1:
- fixed stmmac_probe_config_dt() for !CONFIG_OF
- added missing queue in patch subject

Michael Walle (2):
of: net: pass the dst buffer to of_get_mac_address()
of: net: fix of_get_mac_addr_nvmem() for non-platform devices

arch/arm/mach-mvebu/kirkwood.c | 3 +-
arch/powerpc/sysdev/tsi108_dev.c | 5 +-
drivers/net/ethernet/aeroflex/greth.c | 6 +-
drivers/net/ethernet/allwinner/sun4i-emac.c | 10 +--
drivers/net/ethernet/altera/altera_tse_main.c | 7 +-
drivers/net/ethernet/arc/emac_main.c | 8 +-
drivers/net/ethernet/atheros/ag71xx.c | 7 +-
drivers/net/ethernet/broadcom/bcm4908_enet.c | 7 +-
drivers/net/ethernet/broadcom/bcmsysport.c | 7 +-
drivers/net/ethernet/broadcom/bgmac-bcma.c | 10 +--
.../net/ethernet/broadcom/bgmac-platform.c | 11 ++-
drivers/net/ethernet/cadence/macb_main.c | 11 +--
.../net/ethernet/cavium/octeon/octeon_mgmt.c | 8 +-
.../net/ethernet/cavium/thunder/thunder_bgx.c | 5 +-
drivers/net/ethernet/davicom/dm9000.c | 10 +--
drivers/net/ethernet/ethoc.c | 6 +-
drivers/net/ethernet/ezchip/nps_enet.c | 7 +-
drivers/net/ethernet/freescale/fec_main.c | 7 +-
drivers/net/ethernet/freescale/fec_mpc52xx.c | 7 +-
drivers/net/ethernet/freescale/fman/mac.c | 9 +-
.../ethernet/freescale/fs_enet/fs_enet-main.c | 5 +-
drivers/net/ethernet/freescale/gianfar.c | 8 +-
drivers/net/ethernet/freescale/ucc_geth.c | 5 +-
drivers/net/ethernet/hisilicon/hisi_femac.c | 7 +-
drivers/net/ethernet/hisilicon/hix5hd2_gmac.c | 7 +-
drivers/net/ethernet/lantiq_xrx200.c | 7 +-
drivers/net/ethernet/marvell/mv643xx_eth.c | 5 +-
drivers/net/ethernet/marvell/mvneta.c | 6 +-
.../ethernet/marvell/prestera/prestera_main.c | 11 +--
drivers/net/ethernet/marvell/pxa168_eth.c | 9 +-
drivers/net/ethernet/marvell/sky2.c | 8 +-
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 11 +--
drivers/net/ethernet/micrel/ks8851_common.c | 7 +-
drivers/net/ethernet/microchip/lan743x_main.c | 5 +-
drivers/net/ethernet/nxp/lpc_eth.c | 4 +-
drivers/net/ethernet/qualcomm/qca_spi.c | 10 +--
drivers/net/ethernet/qualcomm/qca_uart.c | 9 +-
drivers/net/ethernet/renesas/ravb_main.c | 12 +--
drivers/net/ethernet/renesas/sh_eth.c | 5 +-
.../ethernet/samsung/sxgbe/sxgbe_platform.c | 13 +--
drivers/net/ethernet/socionext/sni_ave.c | 10 +--
.../ethernet/stmicro/stmmac/dwmac-anarion.c | 2 +-
.../stmicro/stmmac/dwmac-dwc-qos-eth.c | 2 +-
.../ethernet/stmicro/stmmac/dwmac-generic.c | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac-imx.c | 2 +-
.../stmicro/stmmac/dwmac-intel-plat.c | 2 +-
.../ethernet/stmicro/stmmac/dwmac-ipq806x.c | 2 +-
.../ethernet/stmicro/stmmac/dwmac-lpc18xx.c | 2 +-
.../ethernet/stmicro/stmmac/dwmac-mediatek.c | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac-meson.c | 2 +-
.../ethernet/stmicro/stmmac/dwmac-meson8b.c | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac-oxnas.c | 2 +-
.../stmicro/stmmac/dwmac-qcom-ethqos.c | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac-rk.c | 2 +-
.../ethernet/stmicro/stmmac/dwmac-socfpga.c | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac-sti.c | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 2 +-
.../net/ethernet/stmicro/stmmac/dwmac-sunxi.c | 2 +-
.../ethernet/stmicro/stmmac/dwmac-visconti.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 +-
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
.../ethernet/stmicro/stmmac/stmmac_platform.c | 14 +--
.../ethernet/stmicro/stmmac/stmmac_platform.h | 2 +-
drivers/net/ethernet/ti/am65-cpsw-nuss.c | 19 ++---
drivers/net/ethernet/ti/cpsw.c | 7 +-
drivers/net/ethernet/ti/cpsw_new.c | 7 +-
drivers/net/ethernet/ti/davinci_emac.c | 8 +-
drivers/net/ethernet/ti/netcp_core.c | 7 +-
drivers/net/ethernet/wiznet/w5100-spi.c | 8 +-
drivers/net/ethernet/wiznet/w5100.c | 2 +-
drivers/net/ethernet/xilinx/ll_temac_main.c | 8 +-
.../net/ethernet/xilinx/xilinx_axienet_main.c | 15 ++--
drivers/net/ethernet/xilinx/xilinx_emaclite.c | 8 +-
drivers/net/wireless/ath/ath9k/init.c | 5 +-
drivers/net/wireless/mediatek/mt76/eeprom.c | 9 +-
.../net/wireless/ralink/rt2x00/rt2x00dev.c | 6 +-
drivers/of/of_net.c | 85 ++++++++++++-------
drivers/staging/octeon/ethernet.c | 10 +--
drivers/staging/wfx/main.c | 7 +-
include/linux/of_net.h | 6 +-
include/net/dsa.h | 2 +-
net/dsa/dsa2.c | 2 +-
net/dsa/slave.c | 2 +-
net/ethernet/eth.c | 11 +--
85 files changed, 243 insertions(+), 364 deletions(-)

--
2.20.1


2021-04-12 17:49:36

by Michael Walle

[permalink] [raw]
Subject: [PATCH net-next v4 2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices

of_get_mac_address() already supports fetching the MAC address by an
nvmem provider. But until now, it was just working for platform devices.
Esp. it was not working for DSA ports and PCI devices. It gets more
common that PCI devices have a device tree binding since SoCs contain
integrated root complexes.

Use the nvmem of_* binding to fetch the nvmem cells by a struct
device_node. We still have to try to read the cell by device first
because there might be a nvmem_cell_lookup associated with that device.

Signed-off-by: Michael Walle <[email protected]>
---
drivers/of/of_net.c | 35 ++++++++++++++++++++++++++++++-----
1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index cb77b774bf76..dbac3a172a11 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -11,6 +11,7 @@
#include <linux/phy.h>
#include <linux/export.h>
#include <linux/device.h>
+#include <linux/nvmem-consumer.h>

/**
* of_get_phy_mode - Get phy mode for given device_node
@@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr)
static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
{
struct platform_device *pdev = of_find_device_by_node(np);
+ struct nvmem_cell *cell;
+ const void *mac;
+ size_t len;
int ret;

- if (!pdev)
- return -ENODEV;
+ /* Try lookup by device first, there might be a nvmem_cell_lookup
+ * associated with a given device.
+ */
+ if (pdev) {
+ ret = nvmem_get_mac_address(&pdev->dev, addr);
+ put_device(&pdev->dev);
+ return ret;
+ }
+
+ cell = of_nvmem_cell_get(np, "mac-address");
+ if (IS_ERR(cell))
+ return PTR_ERR(cell);
+
+ mac = nvmem_cell_read(cell, &len);
+ nvmem_cell_put(cell);
+
+ if (IS_ERR(mac))
+ return PTR_ERR(mac);
+
+ if (len != ETH_ALEN || !is_valid_ether_addr(mac)) {
+ kfree(mac);
+ return -EINVAL;
+ }

- ret = nvmem_get_mac_address(&pdev->dev, addr);
- put_device(&pdev->dev);
+ memcpy(addr, mac, ETH_ALEN);
+ kfree(mac);

- return ret;
+ return 0;
}

/**
--
2.20.1

2021-04-13 07:38:03

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH net-next v4 2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices

On Mon, Apr 12, 2021 at 07:47:18PM +0200, Michael Walle wrote:
> of_get_mac_address() already supports fetching the MAC address by an
> nvmem provider. But until now, it was just working for platform devices.
> Esp. it was not working for DSA ports and PCI devices. It gets more
> common that PCI devices have a device tree binding since SoCs contain
> integrated root complexes.
>
> Use the nvmem of_* binding to fetch the nvmem cells by a struct
> device_node. We still have to try to read the cell by device first
> because there might be a nvmem_cell_lookup associated with that device.
>
> Signed-off-by: Michael Walle <[email protected]>

Reviewed-by: Andrew Lunn <[email protected]>

Andrew

2021-04-14 06:57:39

by patchwork-bot+netdevbpf

[permalink] [raw]
Subject: Re: [PATCH net-next v4 0/2] of: net: support non-platform devices in of_get_mac_address()

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Mon, 12 Apr 2021 19:47:16 +0200 you wrote:
> of_get_mac_address() is commonly used to fetch the MAC address
> from the device tree. It also supports reading it from a NVMEM
> provider. But the latter is only possible for platform devices,
> because only platform devices are searched for a matching device
> node.
>
> Add a second method to fetch the NVMEM cell by a device tree node
> instead of a "struct device".
>
> [...]

Here is the summary with links:
- [net-next,v4,1/2] of: net: pass the dst buffer to of_get_mac_address()
https://git.kernel.org/netdev/net-next/c/83216e3988cd
- [net-next,v4,2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices
https://git.kernel.org/netdev/net-next/c/f10843e04a07

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html


2021-04-16 04:29:04

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH net-next v4 2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices

On Mon, 2021-04-12 at 19:47 +0200, Michael Walle wrote:
>
> /**
> * of_get_phy_mode - Get phy mode for given device_node
> @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np, const char *name, u8 *addr)
> static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
> {
> struct platform_device *pdev = of_find_device_by_node(np);
> + struct nvmem_cell *cell;
> + const void *mac;
> + size_t len;
> int ret;
>
> - if (!pdev)
> - return -ENODEV;
> + /* Try lookup by device first, there might be a nvmem_cell_lookup
> + * associated with a given device.
> + */
> + if (pdev) {
> + ret = nvmem_get_mac_address(&pdev->dev, addr);
> + put_device(&pdev->dev);
> + return ret;
> + }
> +

This smells like the wrong band aid :)

Any struct device can contain an OF node pointer these days.

This seems all backwards. I think we are dealing with bad evolution.

We need to do a lookup for the device because we get passed an of_node.
We should just get passed a device here... or rather stop calling
of_get_mac_addr() from all those drivers and instead call
eth_platform_get_mac_address() which in turns calls of_get_mac_addr().

Then the nvmem stuff gets put in eth_platform_get_mac_address().

of_get_mac_addr() becomes a low-level thingy that most drivers don't
care about.

Cheers,
Ben.


2021-04-16 07:30:53

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH net-next v4 2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices

Am 2021-04-16 05:24, schrieb Benjamin Herrenschmidt:
> On Mon, 2021-04-12 at 19:47 +0200, Michael Walle wrote:
>>
>> /**
>> * of_get_phy_mode - Get phy mode for given device_node
>> @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np,
>> const char *name, u8 *addr)
>> static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
>> {
>> struct platform_device *pdev = of_find_device_by_node(np);
>> + struct nvmem_cell *cell;
>> + const void *mac;
>> + size_t len;
>> int ret;
>>
>> - if (!pdev)
>> - return -ENODEV;
>> + /* Try lookup by device first, there might be a
>> nvmem_cell_lookup
>> + * associated with a given device.
>> + */
>> + if (pdev) {
>> + ret = nvmem_get_mac_address(&pdev->dev, addr);
>> + put_device(&pdev->dev);
>> + return ret;
>> + }
>> +
>
> This smells like the wrong band aid :)
>
> Any struct device can contain an OF node pointer these days.

But not all nodes might have an associated device, see DSA for example.
And as the name suggests of_get_mac_address() operates on a node. So
if a driver calls of_get_mac_address() it should work on the node. What
is wrong IMHO, is that the ethernet drivers where the corresponding
board
has a nvmem_cell_lookup registered is calling of_get_mac_address(node).
It should rather call eth_get_mac_address(dev) in the first place.

One would need to figure out if there is an actual device (with an
assiciated of_node), then call eth_get_mac_address(dev) and if there
isn't a device call of_get_mac_address(node).

But I don't know if that is easy to figure out. Well, one could start
with just the device where nvmem_cell_lookup is used. Then we could
drop the workaround above.

> This seems all backwards. I think we are dealing with bad evolution.
>
> We need to do a lookup for the device because we get passed an of_node.
> We should just get passed a device here... or rather stop calling
> of_get_mac_addr() from all those drivers and instead call
> eth_platform_get_mac_address() which in turns calls of_get_mac_addr().
>
> Then the nvmem stuff gets put in eth_platform_get_mac_address().
>
> of_get_mac_addr() becomes a low-level thingy that most drivers don't
> care about.

The NVMEM thing is just another (optional) way how the MAC address
is fetched from the device tree. Thus, if the drivers have the
of_get_mac_address() call they should automatically get the NVMEM
method, too.

-michael

2021-04-16 15:22:00

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH net-next v4 2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices

On Fri, Apr 16, 2021 at 2:30 AM Michael Walle <[email protected]> wrote:
>
> Am 2021-04-16 05:24, schrieb Benjamin Herrenschmidt:
> > On Mon, 2021-04-12 at 19:47 +0200, Michael Walle wrote:
> >>
> >> /**
> >> * of_get_phy_mode - Get phy mode for given device_node
> >> @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np,
> >> const char *name, u8 *addr)
> >> static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
> >> {
> >> struct platform_device *pdev = of_find_device_by_node(np);
> >> + struct nvmem_cell *cell;
> >> + const void *mac;
> >> + size_t len;
> >> int ret;
> >>
> >> - if (!pdev)
> >> - return -ENODEV;
> >> + /* Try lookup by device first, there might be a
> >> nvmem_cell_lookup
> >> + * associated with a given device.
> >> + */
> >> + if (pdev) {
> >> + ret = nvmem_get_mac_address(&pdev->dev, addr);
> >> + put_device(&pdev->dev);
> >> + return ret;
> >> + }
> >> +
> >
> > This smells like the wrong band aid :)
> >
> > Any struct device can contain an OF node pointer these days.
>
> But not all nodes might have an associated device, see DSA for example.

I believe what Ben is saying and what I said earlier is going from dev
-> OF node is right and OF node -> dev is wrong. If you only have an
OF node, then use an of_* function.

> And as the name suggests of_get_mac_address() operates on a node. So
> if a driver calls of_get_mac_address() it should work on the node. What
> is wrong IMHO, is that the ethernet drivers where the corresponding
> board
> has a nvmem_cell_lookup registered is calling of_get_mac_address(node).
> It should rather call eth_get_mac_address(dev) in the first place.
>
> One would need to figure out if there is an actual device (with an
> assiciated of_node), then call eth_get_mac_address(dev) and if there
> isn't a device call of_get_mac_address(node).

Yes, I think we're all in agreement.

> But I don't know if that is easy to figure out. Well, one could start
> with just the device where nvmem_cell_lookup is used. Then we could
> drop the workaround above.

Start with the ones just passing dev.of_node directly:

$ git grep 'of_get_mac_address(.*of_node)'
drivers/net/ethernet/aeroflex/greth.c: addr =
of_get_mac_address(ofdev->dev.of_node);
drivers/net/ethernet/altera/altera_tse_main.c: macaddr =
of_get_mac_address(pdev->dev.of_node);
drivers/net/ethernet/arc/emac_main.c: mac_addr =
of_get_mac_address(dev->of_node);
drivers/net/ethernet/broadcom/bgmac-bcma.c: mac =
of_get_mac_address(bgmac->dev->of_node);
drivers/net/ethernet/cavium/octeon/octeon_mgmt.c: mac =
of_get_mac_address(pdev->dev.of_node);
drivers/net/ethernet/ethoc.c: mac =
of_get_mac_address(pdev->dev.of_node);
drivers/net/ethernet/ezchip/nps_enet.c: mac_addr =
of_get_mac_address(dev->of_node);
drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c: mac_addr =
of_get_mac_address(ofdev->dev.of_node);
drivers/net/ethernet/marvell/pxa168_eth.c: mac_addr =
of_get_mac_address(pdev->dev.of_node);
drivers/net/ethernet/marvell/sky2.c: iap =
of_get_mac_address(hw->pdev->dev.of_node);
drivers/net/ethernet/mediatek/mtk_eth_soc.c: mac_addr =
of_get_mac_address(mac->of_node);
drivers/net/ethernet/microchip/lan743x_main.c: mac_addr =
of_get_mac_address(pdev->dev.of_node);
drivers/net/ethernet/qualcomm/qca_spi.c: mac =
of_get_mac_address(spi->dev.of_node);
drivers/net/ethernet/qualcomm/qca_uart.c: mac =
of_get_mac_address(serdev->dev.of_node);
drivers/net/ethernet/wiznet/w5100-spi.c: const void *mac =
of_get_mac_address(spi->dev.of_node);
drivers/net/ethernet/xilinx/xilinx_axienet_main.c: mac_addr =
of_get_mac_address(pdev->dev.of_node);
drivers/net/ethernet/xilinx/xilinx_emaclite.c: mac_address =
of_get_mac_address(ofdev->dev.of_node);
drivers/net/wireless/ralink/rt2x00/rt2x00dev.c: mac_addr =
of_get_mac_address(rt2x00dev->dev->of_node);
drivers/staging/octeon/ethernet.c: mac =
of_get_mac_address(priv->of_node);
drivers/staging/wfx/main.c: macaddr =
of_get_mac_address(wdev->dev->of_node);
net/ethernet/eth.c: addr = of_get_mac_address(dev->of_node);

Then this will find most of the rest:
git grep -W 'of_get_mac_address([a-z]*)'| grep -E '(node|np)'

Rob

2021-04-26 10:54:58

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH net-next v4 2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices

Am 2021-04-16 17:19, schrieb Rob Herring:
> On Fri, Apr 16, 2021 at 2:30 AM Michael Walle <[email protected]> wrote:
>>
>> Am 2021-04-16 05:24, schrieb Benjamin Herrenschmidt:
>> > On Mon, 2021-04-12 at 19:47 +0200, Michael Walle wrote:
>> >>
>> >> /**
>> >> * of_get_phy_mode - Get phy mode for given device_node
>> >> @@ -59,15 +60,39 @@ static int of_get_mac_addr(struct device_node *np,
>> >> const char *name, u8 *addr)
>> >> static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
>> >> {
>> >> struct platform_device *pdev = of_find_device_by_node(np);
>> >> + struct nvmem_cell *cell;
>> >> + const void *mac;
>> >> + size_t len;
>> >> int ret;
>> >>
>> >> - if (!pdev)
>> >> - return -ENODEV;
>> >> + /* Try lookup by device first, there might be a
>> >> nvmem_cell_lookup
>> >> + * associated with a given device.
>> >> + */
>> >> + if (pdev) {
>> >> + ret = nvmem_get_mac_address(&pdev->dev, addr);
>> >> + put_device(&pdev->dev);
>> >> + return ret;
>> >> + }
>> >> +
>> >
>> > This smells like the wrong band aid :)
>> >
>> > Any struct device can contain an OF node pointer these days.
>>
>> But not all nodes might have an associated device, see DSA for
>> example.
>
> I believe what Ben is saying and what I said earlier is going from dev
> -> OF node is right and OF node -> dev is wrong. If you only have an
> OF node, then use an of_* function.
>
>> And as the name suggests of_get_mac_address() operates on a node. So
>> if a driver calls of_get_mac_address() it should work on the node.
>> What
>> is wrong IMHO, is that the ethernet drivers where the corresponding
>> board
>> has a nvmem_cell_lookup registered is calling
>> of_get_mac_address(node).
>> It should rather call eth_get_mac_address(dev) in the first place.
>>
>> One would need to figure out if there is an actual device (with an
>> assiciated of_node), then call eth_get_mac_address(dev) and if there
>> isn't a device call of_get_mac_address(node).
>
> Yes, I think we're all in agreement.
>
>> But I don't know if that is easy to figure out. Well, one could start
>> with just the device where nvmem_cell_lookup is used. Then we could
>> drop the workaround above.
>
> Start with the ones just passing dev.of_node directly:
>
> $ git grep 'of_get_mac_address(.*of_node)'

[..]

Before I'll try to come up with a patch for this, I'd like to get
your opinion on it.

(1) replacing of_get_mac_address(node) with eth_get_mac_address(dev)
might sometimes lead to confusing comments like in
drivers/net/ethernet/allwinner/sun4i-emac.c:

/* Read MAC-address from DT */
ret = of_get_mac_address(np, ndev->dev_addr);

Do we live with that or should the new name somehow reflect that
it is taken from the device tree.

(2) What do you think of eth_get_mac_address(ndev). That is, the
second argument is missing and ndev->dev_addr is used.
I'm unsure about it. We'd still need a second function for drivers
which don't write ndev->dev_addr directly, but have some custom
logic in between. OTOH it would be like eth_hw_addr_random(ndev).

-michael

2021-04-27 00:05:14

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH net-next v4 2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices

On Mon, 2021-04-26 at 12:54 +0200, Michael Walle wrote:
> Before I'll try to come up with a patch for this, I'd like to get
> your opinion on it.
>
> (1) replacing of_get_mac_address(node) with eth_get_mac_address(dev)
> might sometimes lead to confusing comments like in
> drivers/net/ethernet/allwinner/sun4i-emac.c:
>
> /* Read MAC-address from DT */
> ret = of_get_mac_address(np, ndev->dev_addr);

You could leave it or turn it into "from platform", doesn't matter...

> (2) What do you think of eth_get_mac_address(ndev). That is, the

Not sure what you mean, eth_platform_get_mac_address() takes the
address as an argument. I think what you want is a consolidated
nvmem_get_mac_address + eth_platform_get_mac_address that takes a
device, which would have no requirement of the bus_type at all.

Cheers,
Ben.

2021-04-28 08:10:23

by Michael Walle

[permalink] [raw]
Subject: Re: [PATCH net-next v4 2/2] of: net: fix of_get_mac_addr_nvmem() for non-platform devices

Am 2021-04-27 01:44, schrieb Benjamin Herrenschmidt:
> On Mon, 2021-04-26 at 12:54 +0200, Michael Walle wrote:
>> (2) What do you think of eth_get_mac_address(ndev). That is, the
>
> Not sure what you mean, eth_platform_get_mac_address() takes the
> address as an argument. I think what you want is a consolidated
> nvmem_get_mac_address + eth_platform_get_mac_address that takes a
> device, which would have no requirement of the bus_type at all.

Sure. What I meant was the following:

eth_get_mac_address(struct net_device *ndev)
vs.
eth_get_mac_address(struct device *dev, u8 *mac_buf)

The first would assume the destination is ndev->dev_addr (which
is true for most of the calls, but not all).

-michael