mbox series

[v2,00/29] at24: remove at24_platform_data

Message ID 20180810080526.27207-1-brgl@bgdev.pl (mailing list archive)
Headers show
Series at24: remove at24_platform_data | expand

Message

Bartosz Golaszewski Aug. 10, 2018, 8:04 a.m. UTC
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This is a follow-up to the previously rejected series[1] which partially
removed the at24_platform_data structure. After further development and
taking reviews into account, this series finally removes that struct
completely but not without touching many different parts of the code
base.

Since I took over maintainership of the at24 driver I've been working
towards removing at24_platform_data in favor for device properties.

DaVinci is the only platform that's still using it - all other users
have already been converted.

One of the obstacles in case of DaVinci is removing the setup() callback
from the pdata struct, the only user of which are some davinci boards.

Most boards use the EEPROM to store the MAC address. This series adds
support for cell lookups to the nvmem framework, registers relevant
cells for all users, adds nvmem support to eth_platform_get_mac_address(),
converts davinci_emac driver to using it and replaces at24_platform_data
with device properties.

There's also one board (da850-evm) which uses MTD for reading the MAC
address. I used the patch from Alban Bedel's previous submission[2] to
add support for nvmem to the MTD framework. Since this user doesn't
need device tree, I dropped Alban's patches modifying the DT bindings.
We can add that later once an agreement is reached. For the time being
MTD devices are registered as nvmem devices and we're registering the
mac-address cell using the cell lookup mechanism.

This series adds a blocking notifier chain to the nvmem framework, so
that we can keep the EEPROM reading code in the mityomapl138 board file
with only slight modifications.

I also included some minor fixes to the modified code.

Tested on da850-evm & dm365-evm.

[1] https://lkml.org/lkml/2018/6/29/153
[2] https://lkml.org/lkml/2018/3/24/312

v1 -> v2:

  PATCH 4/29:
  - change name to nvmem_dev_name()
  - return NULL instead of ERR_PTR(-ENOSYS) from nvmem_dev_name() if nvmem is
    not configured

  PATCH 5/29:
  - s/nvmem_device_name/nvmem_dev_name/

  PATCH 6/29:
  - fix the commit message: remove the part mentioning the OF systems

  PATCH 7/29:
  - fix ordering of includes

  PATCH 8/29:
  - fix ordering of includes

  PATCH 9/29:
  - fix ordering of includes

  PATCH 10/29:
  - fix ordering of includes

  PATCH 11/29:
  - fix ordering of includes

  PATCH 14/29:
  - new patch

  PATCH 22/29:
  - added missing terminator

  PATCH 25/29:
  - removed unneeded coma

  PATCH 27/29:
  - s/nvmem_device_name/nvmem_dev_name/
  - fix ordering of includes

  PATCH 28/29:
  - added missing terminator

Alban Bedel (1):
  mtd: Add support for reading MTD devices via the nvmem API

Bartosz Golaszewski (28):
  nvmem: add support for cell lookups
  Documentation: nvmem: document lookup entries
  nvmem: add a notifier chain
  nvmem: provide nvmem_dev_name()
  nvmem: remove the name field from struct nvmem_device
  ARM: davinci: dm365-evm: use nvmem lookup for mac address
  ARM: davinci: dm644-evm: use nvmem lookup for mac address
  ARM: davinci: dm646x-evm: use nvmem lookup for mac address
  ARM: davinci: da830-evm: use nvmem lookup for mac address
  ARM: davinci: mityomapl138: add nvmem cells lookup entries
  ARM: davinci: da850-evm: use nvmem lookup for mac address
  ARM: davinci: da850-evm: remove unnecessary include
  net: simplify eth_platform_get_mac_address()
  net: split eth_platform_get_mac_address() into subroutines
  net: add support for nvmem to eth_platform_get_mac_address()
  net: davinci_emac: use eth_platform_get_mac_address()
  ARM: davinci: da850-evm: remove dead MTD code
  ARM: davinci: mityomapl138: don't read the MAC address from machine
    code
  ARM: davinci: dm365-evm: use device properties for at24 eeprom
  ARM: davinci: da830-evm: use device properties for at24 eeprom
  ARM: davinci: dm644x-evm: use device properties for at24 eeprom
  ARM: davinci: dm646x-evm: use device properties for at24 eeprom
  ARM: davinci: sffsdr: fix the at24 eeprom device name
  ARM: davinci: sffsdr: use device properties for at24 eeprom
  ARM: davinci: remove dead code related to MAC address reading
  ARM: davinci: mityomapl138: use nvmem notifiers
  ARM: davinci: mityomapl138: use device properties for at24 eeprom
  eeprom: at24: kill at24_platform_data

 Documentation/nvmem/nvmem.txt              |  28 +++++
 MAINTAINERS                                |   1 -
 arch/arm/mach-davinci/board-da830-evm.c    |  25 ++--
 arch/arm/mach-davinci/board-da850-evm.c    |  45 +++-----
 arch/arm/mach-davinci/board-dm365-evm.c    |  25 ++--
 arch/arm/mach-davinci/board-dm644x-evm.c   |  25 ++--
 arch/arm/mach-davinci/board-dm646x-evm.c   |  25 ++--
 arch/arm/mach-davinci/board-mityomapl138.c |  60 +++++++---
 arch/arm/mach-davinci/board-sffsdr.c       |  13 +--
 arch/arm/mach-davinci/common.c             |  15 ---
 drivers/misc/eeprom/at24.c                 | 127 +++++++++------------
 drivers/mtd/Kconfig                        |   1 +
 drivers/mtd/mtdcore.c                      |  50 ++++++++
 drivers/net/ethernet/ti/davinci_emac.c     |  12 +-
 drivers/nvmem/core.c                       | 106 ++++++++++++++++-
 include/linux/davinci_emac.h               |   2 -
 include/linux/mtd/mtd.h                    |   2 +
 include/linux/nvmem-consumer.h             |  31 +++++
 include/linux/nvmem-provider.h             |  10 ++
 include/linux/platform_data/at24.h         |  60 ----------
 net/ethernet/eth.c                         |  84 ++++++++++++--
 21 files changed, 492 insertions(+), 255 deletions(-)
 delete mode 100644 include/linux/platform_data/at24.h

Comments

Srinivas Kandagatla Aug. 10, 2018, 8:41 a.m. UTC | #1
On 10/08/18 09:04, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> This is a follow-up to the previously rejected series[1] which partially
> removed the at24_platform_data structure. After further development and
> taking reviews into account, this series finally removes that struct
> completely but not without touching many different parts of the code
> base.
> 
> Since I took over maintainership of the at24 driver I've been working
> towards removing at24_platform_data in favor for device properties.
> 
> DaVinci is the only platform that's still using it - all other users
> have already been converted.
> 
> One of the obstacles in case of DaVinci is removing the setup() callback
> from the pdata struct, the only user of which are some davinci boards.
> 
> Most boards use the EEPROM to store the MAC address. This series adds
> support for cell lookups to the nvmem framework, registers relevant
> cells for all users, adds nvmem support to eth_platform_get_mac_address(),
> converts davinci_emac driver to using it and replaces at24_platform_data
> with device properties.
> 
> There's also one board (da850-evm) which uses MTD for reading the MAC
> address. I used the patch from Alban Bedel's previous submission[2] to
> add support for nvmem to the MTD framework. Since this user doesn't
> need device tree, I dropped Alban's patches modifying the DT bindings.
> We can add that later once an agreement is reached. For the time being
> MTD devices are registered as nvmem devices and we're registering the
> mac-address cell using the cell lookup mechanism.
> 
> This series adds a blocking notifier chain to the nvmem framework, so
> that we can keep the EEPROM reading code in the mityomapl138 board file
> with only slight modifications.
> 
> I also included some minor fixes to the modified code.
> 
> Tested on da850-evm & dm365-evm.
> 
> [1] https://lkml.org/lkml/2018/6/29/153
> [2] https://lkml.org/lkml/2018/3/24/312
> 
...

> Bartosz Golaszewski (28):
>    nvmem: add support for cell lookups
>    Documentation: nvmem: document lookup entries
>    nvmem: add a notifier chain
>    nvmem: provide nvmem_dev_name()
>    nvmem: remove the name field from struct nvmem_device
> 
>   Documentation/nvmem/nvmem.txt              |  28 +++++
..
>   drivers/nvmem/core.c                       | 106 ++++++++++++++++-

nvmem parts looks good to me other then few trivial kernel doc comments!
I can either Ack those patches or Send them after rc3-4 to Greg KH as 
4.20 material.

Thanks
srini

>
Brian Norris Aug. 31, 2018, 7:46 p.m. UTC | #2
Hi,

On Fri, Aug 10, 2018 at 10:04:57AM +0200, Bartosz Golaszewski wrote:
> Most boards use the EEPROM to store the MAC address. This series adds
> support for cell lookups to the nvmem framework, registers relevant
> cells for all users, adds nvmem support to eth_platform_get_mac_address(),
> converts davinci_emac driver to using it and replaces at24_platform_data
> with device properties.

We already have:

of_get_nvmem_mac_address() (which does exactly what you're adding,
except it's DT specific)
of_get_mac_address()
fwnode_get_mac_address()
device_get_mac_address()

and now you've taught me that this exists too:

eth_platform_get_mac_address()

These mostly don't share code, and with your series, they'll start to
diverge even more as to what they support. Can you please help rectify
that, instead of widening the gap?

For instance, you can delete most of eth_platform_get_mac_address() and
replace it with device_get_mac_address() [1]. And you could add your new
stuff to fwnode_get_mac_address().

And important part to note here is that you code isn't just useful for
ethernet -- it could be useful for Wifi devices too. So IMO, sticking it
only in an "eth" function is the wrong move.

Brian

[1] arch_get_platform_mac_address() is the only part I wouldn't want to
replicate into a truly generic helper. The following should be a no-op
refactor, AIUI:

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index ee28440f57c5..b738651f71e1 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -47,12 +47,12 @@
 #include <linux/inet.h>
 #include <linux/ip.h>
 #include <linux/netdevice.h>
+#include <linux/property.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/if_ether.h>
-#include <linux/of_net.h>
 #include <linux/pci.h>
 #include <net/dst.h>
 #include <net/arp.h>
@@ -528,19 +528,11 @@ unsigned char * __weak arch_get_platform_mac_address(void)
 int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
 {
 	const unsigned char *addr;
-	struct device_node *dp;
 
-	if (dev_is_pci(dev))
-		dp = pci_device_to_OF_node(to_pci_dev(dev));
-	else
-		dp = dev->of_node;
-
-	addr = NULL;
-	if (dp)
-		addr = of_get_mac_address(dp);
-	if (!addr)
-		addr = arch_get_platform_mac_address();
+	if (device_get_mac_address(dev, mac_addr, ETH_ALEN))
+		return 0;
 
+	addr = arch_get_platform_mac_address();
 	if (!addr)
 		return -ENODEV;
Bartosz Golaszewski Oct. 3, 2018, 8:15 p.m. UTC | #3
pt., 31 sie 2018 o 21:46 Brian Norris <computersforpeace@gmail.com> napisał(a):
>
> Hi,
>
> On Fri, Aug 10, 2018 at 10:04:57AM +0200, Bartosz Golaszewski wrote:
> > Most boards use the EEPROM to store the MAC address. This series adds
> > support for cell lookups to the nvmem framework, registers relevant
> > cells for all users, adds nvmem support to eth_platform_get_mac_address(),
> > converts davinci_emac driver to using it and replaces at24_platform_data
> > with device properties.
>
> We already have:
>
> of_get_nvmem_mac_address() (which does exactly what you're adding,
> except it's DT specific)
> of_get_mac_address()
> fwnode_get_mac_address()
> device_get_mac_address()
>
> and now you've taught me that this exists too:
>
> eth_platform_get_mac_address()
>
> These mostly don't share code, and with your series, they'll start to
> diverge even more as to what they support. Can you please help rectify
> that, instead of widening the gap?
>
> For instance, you can delete most of eth_platform_get_mac_address() and
> replace it with device_get_mac_address() [1]. And you could add your new
> stuff to fwnode_get_mac_address().
>
> And important part to note here is that you code isn't just useful for
> ethernet -- it could be useful for Wifi devices too. So IMO, sticking it
> only in an "eth" function is the wrong move.
>
> Brian
>
> [1] arch_get_platform_mac_address() is the only part I wouldn't want to
> replicate into a truly generic helper. The following should be a no-op
> refactor, AIUI:
>

The only user of arch_get_platform_mac_address() is sparc. It returns
an address that seems to be read from some kind of EEPROM. I'm not
familiar with this arch though. I'm wondering if we could somehow
seamlessly remove this call and then convert all users of
eth_platform_get_mac_address() to using device_get_mac_address()?

David: I couldn't find a place in sparc code where any ethernet device
would be registered, so is there a chance that nobody is using it?

Best regards,
Bartosz Golaszewski

> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index ee28440f57c5..b738651f71e1 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -47,12 +47,12 @@
>  #include <linux/inet.h>
>  #include <linux/ip.h>
>  #include <linux/netdevice.h>
> +#include <linux/property.h>
>  #include <linux/etherdevice.h>
>  #include <linux/skbuff.h>
>  #include <linux/errno.h>
>  #include <linux/init.h>
>  #include <linux/if_ether.h>
> -#include <linux/of_net.h>
>  #include <linux/pci.h>
>  #include <net/dst.h>
>  #include <net/arp.h>
> @@ -528,19 +528,11 @@ unsigned char * __weak arch_get_platform_mac_address(void)
>  int eth_platform_get_mac_address(struct device *dev, u8 *mac_addr)
>  {
>         const unsigned char *addr;
> -       struct device_node *dp;
>
> -       if (dev_is_pci(dev))
> -               dp = pci_device_to_OF_node(to_pci_dev(dev));
> -       else
> -               dp = dev->of_node;
> -
> -       addr = NULL;
> -       if (dp)
> -               addr = of_get_mac_address(dp);
> -       if (!addr)
> -               addr = arch_get_platform_mac_address();
> +       if (device_get_mac_address(dev, mac_addr, ETH_ALEN))
> +               return 0;
>
> +       addr = arch_get_platform_mac_address();
>         if (!addr)
>                 return -ENODEV;
>
Lukas Wunner Oct. 3, 2018, 8:30 p.m. UTC | #4
On Wed, Oct 03, 2018 at 10:15:23PM +0200, Bartosz Golaszewski wrote:
> The only user of arch_get_platform_mac_address() is sparc. It returns
> an address that seems to be read from some kind of EEPROM. I'm not
> familiar with this arch though. I'm wondering if we could somehow
> seamlessly remove this call and then convert all users of
> eth_platform_get_mac_address() to using device_get_mac_address()?

My recollection is that '90s SparcStations had a battery-backed memory
containing the MAC address.  When the battery wore out, the machine
would fail to boot because the address was either all ones or all zeros.
I recall setting the MAC address in that memory using Forth from the
boot prompt.  Ah, the good old days!
Florian Fainelli Oct. 3, 2018, 9:04 p.m. UTC | #5
On 10/3/2018 1:15 PM, Bartosz Golaszewski wrote:
> pt., 31 sie 2018 o 21:46 Brian Norris <computersforpeace@gmail.com> napisał(a):
>>
>> Hi,
>>
>> On Fri, Aug 10, 2018 at 10:04:57AM +0200, Bartosz Golaszewski wrote:
>>> Most boards use the EEPROM to store the MAC address. This series adds
>>> support for cell lookups to the nvmem framework, registers relevant
>>> cells for all users, adds nvmem support to eth_platform_get_mac_address(),
>>> converts davinci_emac driver to using it and replaces at24_platform_data
>>> with device properties.
>>
>> We already have:
>>
>> of_get_nvmem_mac_address() (which does exactly what you're adding,
>> except it's DT specific)
>> of_get_mac_address()
>> fwnode_get_mac_address()
>> device_get_mac_address()
>>
>> and now you've taught me that this exists too:
>>
>> eth_platform_get_mac_address()
>>
>> These mostly don't share code, and with your series, they'll start to
>> diverge even more as to what they support. Can you please help rectify
>> that, instead of widening the gap?
>>
>> For instance, you can delete most of eth_platform_get_mac_address() and
>> replace it with device_get_mac_address() [1]. And you could add your new
>> stuff to fwnode_get_mac_address().
>>
>> And important part to note here is that you code isn't just useful for
>> ethernet -- it could be useful for Wifi devices too. So IMO, sticking it
>> only in an "eth" function is the wrong move.
>>
>> Brian
>>
>> [1] arch_get_platform_mac_address() is the only part I wouldn't want to
>> replicate into a truly generic helper. The following should be a no-op
>> refactor, AIUI:
>>
> 
> The only user of arch_get_platform_mac_address() is sparc. It returns
> an address that seems to be read from some kind of EEPROM. I'm not
> familiar with this arch though. I'm wondering if we could somehow
> seamlessly remove this call and then convert all users of
> eth_platform_get_mac_address() to using device_get_mac_address()?
> 
> David: I couldn't find a place in sparc code where any ethernet device
> would be registered, so is there a chance that nobody is using it?

SPARC uses a true Open Firmware implementation, so it would register 
drivers through the CONFIG_OF infrastructure.
Bartosz Golaszewski Oct. 4, 2018, 11:06 a.m. UTC | #6
śr., 3 paź 2018 o 23:04 Florian Fainelli <f.fainelli@gmail.com> napisał(a):
>
>
>
> On 10/3/2018 1:15 PM, Bartosz Golaszewski wrote:
> > pt., 31 sie 2018 o 21:46 Brian Norris <computersforpeace@gmail.com> napisał(a):
> >>
> >> Hi,
> >>
> >> On Fri, Aug 10, 2018 at 10:04:57AM +0200, Bartosz Golaszewski wrote:
> >>> Most boards use the EEPROM to store the MAC address. This series adds
> >>> support for cell lookups to the nvmem framework, registers relevant
> >>> cells for all users, adds nvmem support to eth_platform_get_mac_address(),
> >>> converts davinci_emac driver to using it and replaces at24_platform_data
> >>> with device properties.
> >>
> >> We already have:
> >>
> >> of_get_nvmem_mac_address() (which does exactly what you're adding,
> >> except it's DT specific)
> >> of_get_mac_address()
> >> fwnode_get_mac_address()
> >> device_get_mac_address()
> >>
> >> and now you've taught me that this exists too:
> >>
> >> eth_platform_get_mac_address()
> >>
> >> These mostly don't share code, and with your series, they'll start to
> >> diverge even more as to what they support. Can you please help rectify
> >> that, instead of widening the gap?
> >>
> >> For instance, you can delete most of eth_platform_get_mac_address() and
> >> replace it with device_get_mac_address() [1]. And you could add your new
> >> stuff to fwnode_get_mac_address().
> >>
> >> And important part to note here is that you code isn't just useful for
> >> ethernet -- it could be useful for Wifi devices too. So IMO, sticking it
> >> only in an "eth" function is the wrong move.
> >>
> >> Brian
> >>
> >> [1] arch_get_platform_mac_address() is the only part I wouldn't want to
> >> replicate into a truly generic helper. The following should be a no-op
> >> refactor, AIUI:
> >>
> >
> > The only user of arch_get_platform_mac_address() is sparc. It returns
> > an address that seems to be read from some kind of EEPROM. I'm not
> > familiar with this arch though. I'm wondering if we could somehow
> > seamlessly remove this call and then convert all users of
> > eth_platform_get_mac_address() to using device_get_mac_address()?
> >
> > David: I couldn't find a place in sparc code where any ethernet device
> > would be registered, so is there a chance that nobody is using it?
>
> SPARC uses a true Open Firmware implementation, so it would register
> drivers through the CONFIG_OF infrastructure.
> --

I'm seeing that there are only six callers of
eth_platform_get_mac_address() (the only function which calls
arch_get_platform_mac_address()).

Of these six callers four are intel ethernet drivers and two are usb
ethernet adapter drivers.

Is it even possible that sparc wants to get the mac address for a usb
adapter from some memory chip? Maybe we *can* safely remove that
function completely? That would allow us to simplify a lot of code.

Bart
Arnd Bergmann Oct. 4, 2018, 1:58 p.m. UTC | #7
On Thu, Oct 4, 2018 at 1:06 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> śr., 3 paź 2018 o 23:04 Florian Fainelli <f.fainelli@gmail.com> napisał(a):
> > On 10/3/2018 1:15 PM, Bartosz Golaszewski wrote:
> > > pt., 31 sie 2018 o 21:46 Brian Norris <computersforpeace@gmail.com> napisał(a):
> > >>
> > >> Hi,
> > >>
> > >> On Fri, Aug 10, 2018 at 10:04:57AM +0200, Bartosz Golaszewski wrote:
> > >>> Most boards use the EEPROM to store the MAC address. This series adds
> > >>> support for cell lookups to the nvmem framework, registers relevant
> > >>> cells for all users, adds nvmem support to eth_platform_get_mac_address(),
> > >>> converts davinci_emac driver to using it and replaces at24_platform_data
> > >>> with device properties.
> > >>
> > >> We already have:
> > >>
> > >> of_get_nvmem_mac_address() (which does exactly what you're adding,
> > >> except it's DT specific)
> > >> of_get_mac_address()
> > >> fwnode_get_mac_address()
> > >> device_get_mac_address()
> > >>
> > >> and now you've taught me that this exists too:
> > >>
> > >> eth_platform_get_mac_address()
> > >>
> > >> These mostly don't share code, and with your series, they'll start to
> > >> diverge even more as to what they support. Can you please help rectify
> > >> that, instead of widening the gap?
> > >>
> > >> For instance, you can delete most of eth_platform_get_mac_address() and
> > >> replace it with device_get_mac_address() [1]. And you could add your new
> > >> stuff to fwnode_get_mac_address().
> > >>
> > >> And important part to note here is that you code isn't just useful for
> > >> ethernet -- it could be useful for Wifi devices too. So IMO, sticking it
> > >> only in an "eth" function is the wrong move.
> > >>
> > >> Brian
> > >>
> > >> [1] arch_get_platform_mac_address() is the only part I wouldn't want to
> > >> replicate into a truly generic helper. The following should be a no-op
> > >> refactor, AIUI:
> > >>
> > >
> > > The only user of arch_get_platform_mac_address() is sparc. It returns
> > > an address that seems to be read from some kind of EEPROM. I'm not
> > > familiar with this arch though. I'm wondering if we could somehow
> > > seamlessly remove this call and then convert all users of
> > > eth_platform_get_mac_address() to using device_get_mac_address()?
> > >
> > > David: I couldn't find a place in sparc code where any ethernet device
> > > would be registered, so is there a chance that nobody is using it?
> >
> > SPARC uses a true Open Firmware implementation, so it would register
> > drivers through the CONFIG_OF infrastructure.
> > --
>
> I'm seeing that there are only six callers of
> eth_platform_get_mac_address() (the only function which calls
> arch_get_platform_mac_address()).
>
> Of these six callers four are intel ethernet drivers and two are usb
> ethernet adapter drivers.
>
> Is it even possible that sparc wants to get the mac address for a usb
> adapter from some memory chip? Maybe we *can* safely remove that
> function completely? That would allow us to simplify a lot of code.

The calls are not even that old, and clearly added intentionally for sparc,
see commit ba94272d08a7 ("i40e: use eth_platform_get_mac_address()")
which added the first one.

Before that commit, the driver did the same as a couple of sun
specific ones that access the idprom directly:

drivers/net/ethernet/aeroflex/greth.c:
macaddr[i] = (unsigned int) idprom->id_ethaddr[i];
drivers/net/ethernet/amd/sun3lance.c:        dev->dev_addr[i] =
idprom->id_ethaddr[i];
drivers/net/ethernet/amd/sunlance.c:            dev->dev_addr[i] =
idprom->id_ethaddr[i];
drivers/net/ethernet/broadcom/tg3.c:    memcpy(dev->dev_addr,
idprom->id_ethaddr, ETH_ALEN);
drivers/net/ethernet/i825xx/sun3_82586.c:            dev->dev_addr[i]
= idprom->id_ethaddr[i];
drivers/net/ethernet/sun/sunbmac.c:             dev->dev_addr[i] =
idprom->id_ethaddr[i];
drivers/net/ethernet/sun/sungem.c:              addr = idprom->id_ethaddr;
drivers/net/ethernet/sun/sunhme.c:
memcpy(dev->dev_addr, idprom->id_ethaddr, ETH_ALEN);
drivers/net/ethernet/sun/sunhme.c:
memcpy(dev->dev_addr, idprom->id_ethaddr, ETH_ALEN);
drivers/net/ethernet/sun/sunqe.c:       memcpy(dev->dev_addr,
idprom->id_ethaddr, ETH_ALEN);

      Arnd
Sowmini Varadhan Oct. 4, 2018, 2:35 p.m. UTC | #8
Just catching up on this thread, so please excuse any unintentional
misquotes here.

> > > > David: I couldn't find a place in sparc code where any ethernet device
> > > > would be registered, so is there a chance that nobody is using it?
> > >
> > > SPARC uses a true Open Firmware implementation, so it would register
> > > drivers through the CONFIG_OF infrastructure.

correct

> The calls are not even that old, and clearly added intentionally for sparc,
> see commit ba94272d08a7 ("i40e: use eth_platform_get_mac_address()")
> which added the first one.

Yes, correct again. Wouldn't PPC also end up doing the same thing?

See also commit c762dff24c06 (for ixgbe) - without this fix sparc systems
will come up with a bogus mac address and then you end up having to
manually fix this in ugly ways.

--Sowmini
Arnd Bergmann Oct. 4, 2018, 2:38 p.m. UTC | #9
On Thu, Oct 4, 2018 at 4:36 PM Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
>
> Just catching up on this thread, so please excuse any unintentional
> misquotes here.
>
> > > > > David: I couldn't find a place in sparc code where any ethernet device
> > > > > would be registered, so is there a chance that nobody is using it?
> > > >
> > > > SPARC uses a true Open Firmware implementation, so it would register
> > > > drivers through the CONFIG_OF infrastructure.
>
> correct
>
> > The calls are not even that old, and clearly added intentionally for sparc,
> > see commit ba94272d08a7 ("i40e: use eth_platform_get_mac_address()")
> > which added the first one.
>
> Yes, correct again. Wouldn't PPC also end up doing the same thing?
>
> See also commit c762dff24c06 (for ixgbe) - without this fix sparc systems
> will come up with a bogus mac address and then you end up having to
> manually fix this in ugly ways.

The of_get_mac_address() portion is not controversial at all I think,
the question was whether we need the fallback to
arch_get_platform_mac_address() in any of the drivers that
call eth_platform_get_mac_address().

      Arnd