diff mbox series

[net-next,v4,04/14] net: phy: Add a binding for PHY LEDs

Message ID 20230317023125.486-5-ansuelsmth@gmail.com (mailing list archive)
State New, archived
Headers show
Series net: Add basic LED support for switch/phy | expand

Commit Message

Christian Marangi March 17, 2023, 2:31 a.m. UTC
From: Andrew Lunn <andrew@lunn.ch>

Define common binding parsing for all PHY drivers with LEDs using
phylib. Parse the DT as part of the phy_probe and add LEDs to the
linux LED class infrastructure. For the moment, provide a dummy
brightness function, which will later be replaced with a call into the
PHY driver.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/Kconfig      |  1 +
 drivers/net/phy/phy_device.c | 75 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          | 16 ++++++++
 3 files changed, 92 insertions(+)

Comments

Marek Behún March 17, 2023, 7:45 a.m. UTC | #1
On Fri, 17 Mar 2023 03:31:15 +0100
Christian Marangi <ansuelsmth@gmail.com> wrote:

> +	cdev->brightness_set_blocking = phy_led_set_brightness;
> +	cdev->max_brightness = 1;
> +	init_data.devicename = dev_name(&phydev->mdio.dev);
> +	init_data.fwnode = of_fwnode_handle(led);
> +
> +	err = devm_led_classdev_register_ext(dev, cdev, &init_data);

Since init_data.devname_mandatory is false, devicename is ignored.
Which is probably good, becuse the device name of a mdio device is
often ugly, taken from devicetree or switch drivers, for example:
  f1072004.mdio-mii
  fixed-0
  mv88e6xxx-1
So either don't fill devicename or use devname_mandatory (and maybe
fill devicename with something less ugly, but I guess if we don't have
much choice if we want to keep persistent names).

Without devname_mandatory, the name of the LED classdev will be of the
form
  color:function[-function-enumerator],
i.e.
  green:lan
  amber:lan-1

With multiple switch ethenret ports all having LAN function, it is
worth noting that the function enumerator must be explicitly used in the
devicetree, otherwise multiple LEDs will be registered under the same
name, and the LED subsystem will add a number at the and of the name
(function led_classdev_next_name), resulting in names
  green:lan
  green:lan_1
  green:lan_2
  ...
These names are dependent on the order of classdev registration.

Marek
Michal Kubiak March 17, 2023, 1:38 p.m. UTC | #2
On Fri, Mar 17, 2023 at 03:31:15AM +0100, Christian Marangi wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 
> Define common binding parsing for all PHY drivers with LEDs using
> phylib. Parse the DT as part of the phy_probe and add LEDs to the
> linux LED class infrastructure. For the moment, provide a dummy
> brightness function, which will later be replaced with a call into the
> PHY driver.
>

Hi Andrew,

Personally, I see no good reason to provide a dummy implementation
of "phy_led_set_brightness", especially if you implement it in the next
patch. You only use that function only the function pointer in
"led_classdev". I think you can just skip it in this patch.

Please find the rest of my comments inline.

Thanks,
Michal


> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/phy/Kconfig      |  1 +
>  drivers/net/phy/phy_device.c | 75 ++++++++++++++++++++++++++++++++++++
>  include/linux/phy.h          | 16 ++++++++
>  3 files changed, 92 insertions(+)
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index f5df2edc94a5..666efa6b1c8e 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -16,6 +16,7 @@ config PHYLINK
>  menuconfig PHYLIB
>  	tristate "PHY Device support and infrastructure"
>  	depends on NETDEVICES
> +	depends on LEDS_CLASS
>  	select MDIO_DEVICE
>  	select MDIO_DEVRES
>  	help
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 9ba8f973f26f..ee800f93c8c3 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -19,10 +19,12 @@
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
> +#include <linux/list.h>
>  #include <linux/mdio.h>
>  #include <linux/mii.h>
>  #include <linux/mm.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  #include <linux/netdevice.h>
>  #include <linux/phy.h>
>  #include <linux/phy_led_triggers.h>
> @@ -658,6 +660,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
>  	device_initialize(&mdiodev->dev);
>  
>  	dev->state = PHY_DOWN;
> +	INIT_LIST_HEAD(&dev->leds);
>  
>  	mutex_init(&dev->lock);
>  	INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
> @@ -2964,6 +2967,73 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
>  	return phydrv->config_intr && phydrv->handle_interrupt;
>  }
>  
> +/* Dummy implementation until calls into PHY driver are added */
> +static int phy_led_set_brightness(struct led_classdev *led_cdev,
> +				  enum led_brightness value)
> +{
> +	return 0;
> +}

It can be removed from this patch.

> +
> +static int of_phy_led(struct phy_device *phydev,
> +		      struct device_node *led)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct led_init_data init_data = {};
> +	struct led_classdev *cdev;
> +	struct phy_led *phyled;
> +	int err;
> +
> +	phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL);
> +	if (!phyled)
> +		return -ENOMEM;
> +
> +	cdev = &phyled->led_cdev;
> +
> +	err = of_property_read_u32(led, "reg", &phyled->index);
> +	if (err)
> +		return err;

Memory leak. 'phyled' is not freed in case of error.

> +
> +	cdev->brightness_set_blocking = phy_led_set_brightness;

Please move this initialization to the patch where you are actually
implementing this callback.

> +	cdev->max_brightness = 1;
> +	init_data.devicename = dev_name(&phydev->mdio.dev);
> +	init_data.fwnode = of_fwnode_handle(led);
> +
> +	err = devm_led_classdev_register_ext(dev, cdev, &init_data);
> +	if (err)
> +		return err;

Another memory leak.

> +
> +	list_add(&phyled->list, &phydev->leds);

Where do you free the memory allocated for phy_led structure?

> +
> +	return 0;
> +}
> +
> +static int of_phy_leds(struct phy_device *phydev)
> +{
> +	struct device_node *node = phydev->mdio.dev.of_node;
> +	struct device_node *leds, *led;
> +	int err;
> +
> +	if (!IS_ENABLED(CONFIG_OF_MDIO))
> +		return 0;
> +
> +	if (!node)
> +		return 0;
> +
> +	leds = of_get_child_by_name(node, "leds");
> +	if (!leds)
> +		return 0;
> +
> +	for_each_available_child_of_node(leds, led) {
> +		err = of_phy_led(phydev, led);
> +		if (err) {
> +			of_node_put(led);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
>   * @fwnode: pointer to the mdio_device's fwnode
> @@ -3142,6 +3212,11 @@ static int phy_probe(struct device *dev)
>  	/* Set the state to READY by default */
>  	phydev->state = PHY_READY;
>  
> +	/* Get the LEDs from the device tree, and instantiate standard
> +	 * LEDs for them.
> +	 */
> +	err = of_phy_leds(phydev);
> +
>  out:
>  	/* Assert the reset signal */
>  	if (err)
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index fbeba4fee8d4..88a77ff60be9 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -14,6 +14,7 @@
>  #include <linux/compiler.h>
>  #include <linux/spinlock.h>
>  #include <linux/ethtool.h>
> +#include <linux/leds.h>
>  #include <linux/linkmode.h>
>  #include <linux/netlink.h>
>  #include <linux/mdio.h>
> @@ -595,6 +596,7 @@ struct macsec_ops;
>   * @phy_num_led_triggers: Number of triggers in @phy_led_triggers
>   * @led_link_trigger: LED trigger for link up/down
>   * @last_triggered: last LED trigger for link speed
> + * @leds: list of PHY LED structures
>   * @master_slave_set: User requested master/slave configuration
>   * @master_slave_get: Current master/slave advertisement
>   * @master_slave_state: Current master/slave configuration
> @@ -690,6 +692,7 @@ struct phy_device {
>  
>  	struct phy_led_trigger *led_link_trigger;
>  #endif
> +	struct list_head leds;
>  
>  	/*
>  	 * Interrupt number for this PHY
> @@ -825,6 +828,19 @@ struct phy_plca_status {
>  	bool pst;
>  };
>  
> +/**
> + * struct phy_led: An LED driven by the PHY
> + *
> + * @list: List of LEDs
> + * @led_cdev: Standard LED class structure
> + * @index: Number of the LED
> + */
> +struct phy_led {
> +	struct list_head list;
> +	struct led_classdev led_cdev;
> +	u32 index;
> +};
> +
>  /**
>   * struct phy_driver - Driver structure for a particular PHY type
>   *
> -- 
> 2.39.2
>
Andrew Lunn March 17, 2023, 1:55 p.m. UTC | #3
On Fri, Mar 17, 2023 at 08:45:19AM +0100, Marek Behún wrote:
> On Fri, 17 Mar 2023 03:31:15 +0100
> Christian Marangi <ansuelsmth@gmail.com> wrote:
> 
> > +	cdev->brightness_set_blocking = phy_led_set_brightness;
> > +	cdev->max_brightness = 1;
> > +	init_data.devicename = dev_name(&phydev->mdio.dev);
> > +	init_data.fwnode = of_fwnode_handle(led);
> > +
> > +	err = devm_led_classdev_register_ext(dev, cdev, &init_data);
> 
> Since init_data.devname_mandatory is false, devicename is ignored.
> Which is probably good, becuse the device name of a mdio device is
> often ugly, taken from devicetree or switch drivers, for example:
>   f1072004.mdio-mii
>   fixed-0
>   mv88e6xxx-1
> So either don't fill devicename or use devname_mandatory (and maybe
> fill devicename with something less ugly, but I guess if we don't have
> much choice if we want to keep persistent names).
> 
> Without devname_mandatory, the name of the LED classdev will be of the
> form
>   color:function[-function-enumerator],
> i.e.
>   green:lan
>   amber:lan-1
> 
> With multiple switch ethenret ports all having LAN function, it is
> worth noting that the function enumerator must be explicitly used in the
> devicetree, otherwise multiple LEDs will be registered under the same
> name, and the LED subsystem will add a number at the and of the name
> (function led_classdev_next_name), resulting in names
>   green:lan
>   green:lan_1
>   green:lan_2
>   ...

I'm testing on a Marvell RDK, with limited LEDs. It has one LED on the
front port to represent the WAN port. The DT patch is at the end of
the series. With that, i end up with:

root@370rd:/sys/class/leds# ls -l
total 0
lrwxrwxrwx 1 root root 0 Mar 17 01:10 f1072004.mdio-mii:00:WAN -> ../../devices/platform/soc/soc:interna
l-regs/f1072004.mdio/mdio_bus/f1072004.mdio-mii/f1072004.mdio-mii:00/leds/f1072004.mdio-mii:00:WAN

I also have:

root@370rd:/sys/class/net/eth0/phydev/leds# ls
f1072004.mdio-mii:00:WAN

f1072004.mdio-mii:00: is not nice, but it is unique to a netdev. The
last part then comes from the label property. Since there is only one
LED, i went with what the port is intended to be used as. If there had
been more LEDs, i would of probably used labels like "LINK" and
"ACTIVITY", since that is often what they reset default
to. Alternatively, you could names the "Left" and "Right", which does
suggest they can be given any function.

I don't actually think the name is too important, so long as it is
unique. You are going to find it via /sys/class/net. MAC LEDs should
be /sys/class/net/eth42/leds, and PHY LEDs will be
/sys/class/net/phydev/leds.

It has been discussed in the past to either extend ethtool to
understand this, or write a new little tool to make it easier to
manipulate these LEDs.

	   Andrew
Andrew Lunn March 17, 2023, 2:03 p.m. UTC | #4
On Fri, Mar 17, 2023 at 02:38:15PM +0100, Michal Kubiak wrote:
> On Fri, Mar 17, 2023 at 03:31:15AM +0100, Christian Marangi wrote:
> > From: Andrew Lunn <andrew@lunn.ch>
> > 
> > Define common binding parsing for all PHY drivers with LEDs using
> > phylib. Parse the DT as part of the phy_probe and add LEDs to the
> > linux LED class infrastructure. For the moment, provide a dummy
> > brightness function, which will later be replaced with a call into the
> > PHY driver.
> >
> 
> Hi Andrew,
> 
> Personally, I see no good reason to provide a dummy implementation
> of "phy_led_set_brightness", especially if you implement it in the next
> patch. You only use that function only the function pointer in
> "led_classdev". I think you can just skip it in this patch.

Hi Michal

The basic code for this patch has been sitting in my tree for a long
time. It used to be, if you did not have a set_brightness method in
cdev, the registration failed. That made it hard to test this patch on
its own during development work, did i have the link list correct, can
i unload the PHY driver without it exploding etc. I need to check if
it is still mandatory.

> > +static int of_phy_led(struct phy_device *phydev,
> > +		      struct device_node *led)
> > +{
> > +	struct device *dev = &phydev->mdio.dev;
> > +	struct led_init_data init_data = {};
> > +	struct led_classdev *cdev;
> > +	struct phy_led *phyled;
> > +	int err;
> > +
> > +	phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL);
> > +	if (!phyled)
> > +		return -ENOMEM;
> > +
> > +	cdev = &phyled->led_cdev;
> > +
> > +	err = of_property_read_u32(led, "reg", &phyled->index);
> > +	if (err)
> > +		return err;
> 
> Memory leak. 'phyled' is not freed in case of error.

devm_ API, so it gets freed when the probe fails.

> > +
> > +	cdev->brightness_set_blocking = phy_led_set_brightness;
> 
> Please move this initialization to the patch where you are actually
> implementing this callback.
> 
> > +	cdev->max_brightness = 1;
> > +	init_data.devicename = dev_name(&phydev->mdio.dev);
> > +	init_data.fwnode = of_fwnode_handle(led);
> > +
> > +	err = devm_led_classdev_register_ext(dev, cdev, &init_data);
> > +	if (err)
> > +		return err;
> 
> Another memory leak.

Ah, maybe you don't know about devm_ ? devm_ allocations and actions
register an action to be taken when the device is removed, either
because the probe failed, or when the device is unregistered. For
memory allocation, the memory is freed automagically. For actions like
registering an LED, requesting an interrupt etc, an unregister/release
is performed. This makes cleanup less buggy since the core does it.

   Andrew
Marek Behún March 17, 2023, 2:29 p.m. UTC | #5
On Fri, 17 Mar 2023 14:55:11 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Fri, Mar 17, 2023 at 08:45:19AM +0100, Marek Behún wrote:
> > On Fri, 17 Mar 2023 03:31:15 +0100
> > Christian Marangi <ansuelsmth@gmail.com> wrote:
> >   
> > > +	cdev->brightness_set_blocking = phy_led_set_brightness;
> > > +	cdev->max_brightness = 1;
> > > +	init_data.devicename = dev_name(&phydev->mdio.dev);
> > > +	init_data.fwnode = of_fwnode_handle(led);
> > > +
> > > +	err = devm_led_classdev_register_ext(dev, cdev, &init_data);  
> > 
> > Since init_data.devname_mandatory is false, devicename is ignored.
> > Which is probably good, becuse the device name of a mdio device is
> > often ugly, taken from devicetree or switch drivers, for example:
> >   f1072004.mdio-mii
> >   fixed-0
> >   mv88e6xxx-1
> > So either don't fill devicename or use devname_mandatory (and maybe
> > fill devicename with something less ugly, but I guess if we don't have
> > much choice if we want to keep persistent names).
> > 
> > Without devname_mandatory, the name of the LED classdev will be of the
> > form
> >   color:function[-function-enumerator],
> > i.e.
> >   green:lan
> >   amber:lan-1
> > 
> > With multiple switch ethenret ports all having LAN function, it is
> > worth noting that the function enumerator must be explicitly used in the
> > devicetree, otherwise multiple LEDs will be registered under the same
> > name, and the LED subsystem will add a number at the and of the name
> > (function led_classdev_next_name), resulting in names
> >   green:lan
> >   green:lan_1
> >   green:lan_2
> >   ...  
> 
> I'm testing on a Marvell RDK, with limited LEDs. It has one LED on the
> front port to represent the WAN port. The DT patch is at the end of
> the series. With that, i end up with:
> 
> root@370rd:/sys/class/leds# ls -l
> total 0
> lrwxrwxrwx 1 root root 0 Mar 17 01:10 f1072004.mdio-mii:00:WAN -> ../../devices/platform/soc/soc:interna
> l-regs/f1072004.mdio/mdio_bus/f1072004.mdio-mii/f1072004.mdio-mii:00/leds/f1072004.mdio-mii:00:WAN
> 
> I also have:
> 
> root@370rd:/sys/class/net/eth0/phydev/leds# ls
> f1072004.mdio-mii:00:WAN

Hmm, yes I see.  If label is specified, devicename is used even if
devname_mandatory is false.

> f1072004.mdio-mii:00: is not nice, but it is unique to a netdev. The
> last part then comes from the label property. Since there is only one
> LED, i went with what the port is intended to be used as. If there had
> been more LEDs, i would of probably used labels like "LINK" and
> "ACTIVITY", since that is often what they reset default
> to. Alternatively, you could names the "Left" and "Right", which does
> suggest they can be given any function.
> 
> I don't actually think the name is too important, so long as it is
> unique. You are going to find it via /sys/class/net. MAC LEDs should
> be /sys/class/net/eth42/leds, and PHY LEDs will be
> /sys/class/net/phydev/leds.

Maybe the name may not be that important from the perspective of a user
who just wants to find the LED for a given phy, yes, but the
proposal of how LED classdev should be named was done in good faith
and accepted years ago. The documentation still defines the name format
and until that part of documenation is changed, I think we should at
least try to follow it.

Also, the label DT property has been deprecated for LEDs. IMO it should
be removed from that last patch of this series.

> It has been discussed in the past to either extend ethtool to
> understand this, or write a new little tool to make it easier to
> manipulate these LEDs.

Yes, and this would solve the problem for a user who wants to change
the behaviour of a LED for a given PHY. But a user who wants to list
all available LEDs by listing /sys/class/leds can also retrieve a nice
list of names that make sense, if the documented format is followed.

Marek
Michal Kubiak March 17, 2023, 3:12 p.m. UTC | #6
On Fri, Mar 17, 2023 at 03:03:32PM +0100, Andrew Lunn wrote:

> > Hi Andrew,
> > 
> > Personally, I see no good reason to provide a dummy implementation
> > of "phy_led_set_brightness", especially if you implement it in the next
> > patch. You only use that function only the function pointer in
> > "led_classdev". I think you can just skip it in this patch.
> 
> Hi Michal
> 
> The basic code for this patch has been sitting in my tree for a long
> time. It used to be, if you did not have a set_brightness method in
> cdev, the registration failed. That made it hard to test this patch on
> its own during development work, did i have the link list correct, can
> i unload the PHY driver without it exploding etc. I need to check if
> it is still mandatory.
> 

Thank you for the explanation. I was not aware of failing registration
in case of undefined "cdev->brightness_set_blocking". I think it is
a good reason of defining the dummy function. (The only alternative
would be to squash two commits, but I think it is easier to review
smaller chunks of code).


> > > +static int of_phy_led(struct phy_device *phydev,
> > > +		      struct device_node *led)
> > > +{
> > > +	struct device *dev = &phydev->mdio.dev;
> > > +	struct led_init_data init_data = {};
> > > +	struct led_classdev *cdev;
> > > +	struct phy_led *phyled;
> > > +	int err;
> > > +
> > > +	phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL);
> > > +	if (!phyled)
> > > +		return -ENOMEM;
> > > +
> > > +	cdev = &phyled->led_cdev;
> > > +
> > > +	err = of_property_read_u32(led, "reg", &phyled->index);
> > > +	if (err)
> > > +		return err;
> > 
> > Memory leak. 'phyled' is not freed in case of error.
> 
> devm_ API, so it gets freed when the probe fails.
> 
> > > +
> > > +	cdev->brightness_set_blocking = phy_led_set_brightness;
> > 
> > Please move this initialization to the patch where you are actually
> > implementing this callback.
> > 
> > > +	cdev->max_brightness = 1;
> > > +	init_data.devicename = dev_name(&phydev->mdio.dev);
> > > +	init_data.fwnode = of_fwnode_handle(led);
> > > +
> > > +	err = devm_led_classdev_register_ext(dev, cdev, &init_data);
> > > +	if (err)
> > > +		return err;
> > 
> > Another memory leak.
> 
> Ah, maybe you don't know about devm_ ? devm_ allocations and actions
> register an action to be taken when the device is removed, either
> because the probe failed, or when the device is unregistered. For
> memory allocation, the memory is freed automagically. For actions like
> registering an LED, requesting an interrupt etc, an unregister/release
> is performed. This makes cleanup less buggy since the core does it.
> 
>    Andrew


Yeah, it is my fault, I apologize for that.
I didn't consider neither the probe() context, nor the lifetime of the
list. You are right - I had no experience with using this devm_ API,
so I looked at it as a standard memory allocation.
Thank you for your patience and this piece of knowledge.

Thanks,
Michal
Andrew Lunn March 17, 2023, 3:31 p.m. UTC | #7
> Yes, and this would solve the problem for a user who wants to change
> the behaviour of a LED for a given PHY. But a user who wants to list
> all available LEDs by listing /sys/class/leds can also retrieve a nice
> list of names that make sense, if the documented format is followed.

Please make a concrete proposal.

Also, keep in mind, ethernet device names change at runtime, and are
not unique. Function also changes at run time, which is in fact the
whole purpose of this collection of patchsets.

      Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index f5df2edc94a5..666efa6b1c8e 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -16,6 +16,7 @@  config PHYLINK
 menuconfig PHYLIB
 	tristate "PHY Device support and infrastructure"
 	depends on NETDEVICES
+	depends on LEDS_CLASS
 	select MDIO_DEVICE
 	select MDIO_DEVRES
 	help
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9ba8f973f26f..ee800f93c8c3 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -19,10 +19,12 @@ 
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/mdio.h>
 #include <linux/mii.h>
 #include <linux/mm.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/netdevice.h>
 #include <linux/phy.h>
 #include <linux/phy_led_triggers.h>
@@ -658,6 +660,7 @@  struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 	device_initialize(&mdiodev->dev);
 
 	dev->state = PHY_DOWN;
+	INIT_LIST_HEAD(&dev->leds);
 
 	mutex_init(&dev->lock);
 	INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
@@ -2964,6 +2967,73 @@  static bool phy_drv_supports_irq(struct phy_driver *phydrv)
 	return phydrv->config_intr && phydrv->handle_interrupt;
 }
 
+/* Dummy implementation until calls into PHY driver are added */
+static int phy_led_set_brightness(struct led_classdev *led_cdev,
+				  enum led_brightness value)
+{
+	return 0;
+}
+
+static int of_phy_led(struct phy_device *phydev,
+		      struct device_node *led)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct led_init_data init_data = {};
+	struct led_classdev *cdev;
+	struct phy_led *phyled;
+	int err;
+
+	phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL);
+	if (!phyled)
+		return -ENOMEM;
+
+	cdev = &phyled->led_cdev;
+
+	err = of_property_read_u32(led, "reg", &phyled->index);
+	if (err)
+		return err;
+
+	cdev->brightness_set_blocking = phy_led_set_brightness;
+	cdev->max_brightness = 1;
+	init_data.devicename = dev_name(&phydev->mdio.dev);
+	init_data.fwnode = of_fwnode_handle(led);
+
+	err = devm_led_classdev_register_ext(dev, cdev, &init_data);
+	if (err)
+		return err;
+
+	list_add(&phyled->list, &phydev->leds);
+
+	return 0;
+}
+
+static int of_phy_leds(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct device_node *leds, *led;
+	int err;
+
+	if (!IS_ENABLED(CONFIG_OF_MDIO))
+		return 0;
+
+	if (!node)
+		return 0;
+
+	leds = of_get_child_by_name(node, "leds");
+	if (!leds)
+		return 0;
+
+	for_each_available_child_of_node(leds, led) {
+		err = of_phy_led(phydev, led);
+		if (err) {
+			of_node_put(led);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 /**
  * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
  * @fwnode: pointer to the mdio_device's fwnode
@@ -3142,6 +3212,11 @@  static int phy_probe(struct device *dev)
 	/* Set the state to READY by default */
 	phydev->state = PHY_READY;
 
+	/* Get the LEDs from the device tree, and instantiate standard
+	 * LEDs for them.
+	 */
+	err = of_phy_leds(phydev);
+
 out:
 	/* Assert the reset signal */
 	if (err)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index fbeba4fee8d4..88a77ff60be9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -14,6 +14,7 @@ 
 #include <linux/compiler.h>
 #include <linux/spinlock.h>
 #include <linux/ethtool.h>
+#include <linux/leds.h>
 #include <linux/linkmode.h>
 #include <linux/netlink.h>
 #include <linux/mdio.h>
@@ -595,6 +596,7 @@  struct macsec_ops;
  * @phy_num_led_triggers: Number of triggers in @phy_led_triggers
  * @led_link_trigger: LED trigger for link up/down
  * @last_triggered: last LED trigger for link speed
+ * @leds: list of PHY LED structures
  * @master_slave_set: User requested master/slave configuration
  * @master_slave_get: Current master/slave advertisement
  * @master_slave_state: Current master/slave configuration
@@ -690,6 +692,7 @@  struct phy_device {
 
 	struct phy_led_trigger *led_link_trigger;
 #endif
+	struct list_head leds;
 
 	/*
 	 * Interrupt number for this PHY
@@ -825,6 +828,19 @@  struct phy_plca_status {
 	bool pst;
 };
 
+/**
+ * struct phy_led: An LED driven by the PHY
+ *
+ * @list: List of LEDs
+ * @led_cdev: Standard LED class structure
+ * @index: Number of the LED
+ */
+struct phy_led {
+	struct list_head list;
+	struct led_classdev led_cdev;
+	u32 index;
+};
+
 /**
  * struct phy_driver - Driver structure for a particular PHY type
  *