[PATCH 1/2] net: phy: motorcomm: add led_brightness_set for YT8521

[PATCH 1/2] net: phy: motorcomm: add led_brightness_set for YT8521

[Chukun Pan]

Add support for manual brightness setting on YT8521/YT8531 PHY.

Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
---
 drivers/net/phy/motorcomm.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
index 463e1ea3efc9..bcd3afef0d49 100644
--- a/drivers/net/phy/motorcomm.c
+++ b/drivers/net/phy/motorcomm.c
@@ -258,6 +258,10 @@
 #define YTPHY_WCR_INTR_SEL			BIT(6)
 #define YTPHY_WCR_ENABLE			BIT(3)
 
+#define YTPHY_LED_GENERAL_CFG_REG		0xA00B
+#define YTPHY_LED_FORCE_MODE(x)			BIT((x) * 3)
+#define YTPHY_LED_FORCE_EN(x)			BIT((x) * 3 + 2)
+
 /* 2b00 84ms
  * 2b01 168ms  *default*
  * 2b10 336ms
@@ -1775,6 +1779,31 @@ static int yt8531s_config_init(struct phy_device *phydev)
 	return phy_restore_page(phydev, old_page, ret);
 }
 
+static int yt8521_led_brightness_set(struct phy_device *phydev, u8 index,
+				     enum led_brightness value)
+{
+	u16 regs;
+	int ret;
+
+	if (index >= YT8521_MAX_LEDS)
+		return -EINVAL;
+
+	/* clear HW LED setup */
+	ret = ytphy_write_ext(phydev, YT8521_LED0_CFG_REG + index, 0);
+	if (ret < 0)
+		return ret;
+
+	regs = ytphy_read_ext(phydev, YTPHY_LED_GENERAL_CFG_REG);
+	if (value != LED_OFF)
+		regs |= YTPHY_LED_FORCE_EN(index) |
+			YTPHY_LED_FORCE_MODE(index);
+	else
+		regs &= ~(YTPHY_LED_FORCE_EN(index) |
+			  YTPHY_LED_FORCE_MODE(index));
+
+	return ytphy_write_ext(phydev, YTPHY_LED_GENERAL_CFG_REG, regs);
+}
+
 static const unsigned long supported_trgs = (BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
 					     BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
 					     BIT(TRIGGER_NETDEV_LINK)        |
@@ -3266,6 +3295,7 @@ static struct phy_driver motorcomm_phy_drvs[] = {
 		.soft_reset	= yt8521_soft_reset,
 		.suspend	= yt8521_suspend,
 		.resume		= yt8521_resume,
+		.led_brightness_set = yt8521_led_brightness_set,
 		.led_hw_is_supported = yt8521_led_hw_is_supported,
 		.led_hw_control_set = yt8521_led_hw_control_set,
 		.led_hw_control_get = yt8521_led_hw_control_get,
@@ -3288,6 +3318,7 @@ static struct phy_driver motorcomm_phy_drvs[] = {
 		.get_wol	= ytphy_get_wol,
 		.set_wol	= yt8531_set_wol,
 		.link_change_notify = yt8531_link_change_notify,
+		.led_brightness_set = yt8521_led_brightness_set,
 		.led_hw_is_supported = yt8521_led_hw_is_supported,
 		.led_hw_control_set = yt8521_led_hw_control_set,
 		.led_hw_control_get = yt8521_led_hw_control_get,
-- 
2.34.1

[PATCH 2/2] net: phy: motorcomm: add missing trigger link for YT8521

[Chukun Pan]

TRIGGER_NETDEV_LINK is available in led_hw_control_set,
but missing in led_hw_control_get. Add it.

Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
---
 drivers/net/phy/motorcomm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
index bcd3afef0d49..5b1d17bd231a 100644
--- a/drivers/net/phy/motorcomm.c
+++ b/drivers/net/phy/motorcomm.c
@@ -1901,6 +1901,11 @@ static int yt8521_led_hw_control_get(struct phy_device *phydev, u8 index,
 	if (val & YT8521_LED_10_ON_EN)
 		__set_bit(TRIGGER_NETDEV_LINK_10, rules);
 
+	if ((val & YT8521_LED_10_ON_EN) &&
+	    (val & YT8521_LED_100_ON_EN) &&
+	    (val & YT8521_LED_1000_ON_EN))
+		__set_bit(TRIGGER_NETDEV_LINK, rules);
+
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH 2/2] net: phy: motorcomm: add missing trigger link for YT8521

[Andrew Lunn]

On Tue, Nov 10, 2026 at 06:00:01PM +0800, Chukun Pan wrote:
> TRIGGER_NETDEV_LINK is available in led_hw_control_set,
> but missing in led_hw_control_get. Add it.
> 
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH 2/2] net: phy: motorcomm: add missing trigger link for YT8521

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In drivers/net/phy/motorcomm.c, yt8521_led_hw_control_get()
  now sets TRIGGER_NETDEV_LINK when YT8521_LED_10_ON_EN,…
- [Medium] net: phy: motorcomm: bugs in yt8521_led_brightness_set() and
  force mode
- [Low] The patch fixes a user-visible get/set asymmetry: after the
  netdev trigger activates on hardware set up for 'link at any speed',…

--

net: phy: motorcomm: add missing trigger link for YT8521

This patch makes yt8521_led_hw_control_get() report TRIGGER_NETDEV_LINK
when the 10, 100 and 1000 link enable bits are all set in the LED config
register. The goal is to match yt8521_led_hw_control_set(), which already
handles TRIGGER_NETDEV_LINK.

> net: phy: motorcomm: add missing trigger link for YT8521

[Severity: Low]
This isn't a bug, but should this carry a Fixes: tag? The get/set
asymmetry comes from the commit that added the LED callbacks:

Fixes: 355b82c54c12 ("net: phy: motorcomm: Add support for PHY LEDs on YT8521")

Also, the subject only names YT8521. The PHY_ID_YT8531 entry in
motorcomm_phy_drvs[] uses the same callback:

    .led_hw_control_get = yt8521_led_hw_control_get,

so YT8531 behaviour changes too. Could the commit message say so?

> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index bcd3afef0d496..5b1d17bd231a7 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c
> @@ -1901,6 +1901,11 @@ static int yt8521_led_hw_control_get(struct phy_device *phydev, u8 index,
>  	if (val & YT8521_LED_10_ON_EN)
>  		__set_bit(TRIGGER_NETDEV_LINK_10, rules);
>  
> +	if ((val & YT8521_LED_10_ON_EN) &&
> +	    (val & YT8521_LED_100_ON_EN) &&
> +	    (val & YT8521_LED_1000_ON_EN))
> +		__set_bit(TRIGGER_NETDEV_LINK, rules);

[Severity: Medium]
This isn't introduced by this patch, but it is on the same hardware
control path. yt8521_led_brightness_set(), added by the previous patch
in this series, seems to have a few problems.

The return value of ytphy_read_ext() is stored straight into u16 regs:

    regs = ytphy_read_ext(phydev, YTPHY_LED_GENERAL_CFG_REG);

If the MDIO access fails, the negative errno is truncated. The force
bits are applied on top of it and the result is written back to
YTPHY_LED_GENERAL_CFG_REG. Wouldn't that clobber the force enable and
force mode bits of every LED, not just the one at index?

phy_led_set_brightness() only holds phydev->lock around the callback,
but ytphy_read_ext() and ytphy_write_ext() require the caller to hold
the MDIO bus lock. Each access is a YTPHY_PAGE_SELECT write followed
by a YTPHY_PAGE_DATA access. Another user of the bus can change the
page select in between, so the wrong extended register gets read or
written. The read-modify-write of YTPHY_LED_GENERAL_CFG_REG is not
atomic either.

Should this use ytphy_write_ext_with_lock() and
ytphy_modify_ext_with_lock() instead? That would also take care of the
unchecked read above. The ytphy_read_ext() and ytphy_write_ext() calls
in yt8521_led_hw_control_get() and yt8521_led_hw_control_set() look
like they have the same locking issue.

Finally, when value != LED_OFF this sets YTPHY_LED_FORCE_EN(index) in
YTPHY_LED_GENERAL_CFG_REG. But yt8521_led_hw_control_set() only writes
YT8521_LED0_CFG_REG + index and never clears the force bits.

The netdev trigger does not turn the LED off before offloading.
set_baseline_state() calls hw_control_set() directly when
trigger_data->hw_control is true. led_trigger_set() only forces
LED_OFF when a previous trigger is being removed.

So take an LED with trigger none. Writing 1 to brightness and then
selecting the netdev trigger with an offloadable mode would leave the
force bit set. The LED then stays lit regardless of link state or
activity.

Should yt8521_led_hw_control_set() clear YTPHY_LED_FORCE_EN(index) and
YTPHY_LED_FORCE_MODE(index) before programming the rules, similar to
what mxl86110_led_hw_control_set() does?

[Severity: Medium]
TRIGGER_NETDEV_LINK_10, LINK_100 and LINK_1000 are still set just above
this, so the reported mode becomes LINK | LINK_10 | LINK_100 | LINK_1000.
Can the netdev trigger work with that combination?

netdev_trig_activate() stores the result without checking it:

drivers/leds/trigger/ledtrig-netdev.c:netdev_trig_activate() {
    ...
			rc = led_cdev->hw_control_get(led_cdev, &mode);
			if (!rc)
				trigger_data->mode = mode;
    ...
}

But netdev_led_attr_store() treats LINK and the per-speed bits as
mutually exclusive:

drivers/leds/trigger/ledtrig-netdev.c:netdev_led_attr_store() {
    ...
	assign_bit(bit, &mode, state);

	if (test_bit(TRIGGER_NETDEV_LINK, &mode) &&
	    (test_bit(TRIGGER_NETDEV_LINK_10, &mode) ||
	     test_bit(TRIGGER_NETDEV_LINK_100, &mode) ||
	     test_bit(TRIGGER_NETDEV_LINK_1000, &mode) ||
    ...
		return -EINVAL;
    ...
}

Say the trigger activates on an LED whose config register has the
10/100/1000 bits set. That can be the boot default, or the result of an
earlier yt8521_led_hw_control_set() with TRIGGER_NETDEV_LINK, which
writes exactly those three bits.

In that case, wouldn't every sysfs write that leaves LINK set (tx=1,
rx=1, full_duplex=1, link_10=0, even link=1) now fail with -EINVAL?
Only link=0 would get through.

Before this patch, get returned LINK_10 | LINK_100 | LINK_1000, which
is a valid mode, so those writes worked. It also means set(LINK)
followed by get gives back a mode the trigger rejects.

Should get report TRIGGER_NETDEV_LINK instead of the three per-speed
bits when all three are on, rather than in addition to them?

realtek_main.c (rtl8211f, rtl8211e, rtl822x) and air_an8801.c use the
same additive pattern, so they seem to have the same latent conflict.

> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110100001.1873309-1-amadeus%40jmu.edu.cn

Re: [PATCH 1/2] net: phy: motorcomm: add led_brightness_set for YT8521

[Andrew Lunn]

On Tue, Nov 10, 2026 at 06:00:00PM +0800, Chukun Pan wrote:
> Add support for manual brightness setting on YT8521/YT8531 PHY.

Has this been tested on real hardware? Have you had it blinking the
heartbeat? Shift-lock etc.

> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c
> @@ -258,6 +258,10 @@
>  #define YTPHY_WCR_INTR_SEL			BIT(6)
>  #define YTPHY_WCR_ENABLE			BIT(3)
>  
> +#define YTPHY_LED_GENERAL_CFG_REG		0xA00B
> +#define YTPHY_LED_FORCE_MODE(x)			BIT((x) * 3)
> +#define YTPHY_LED_FORCE_EN(x)			BIT((x) * 3 + 2)
> +
>  /* 2b00 84ms
>   * 2b01 168ms  *default*
>   * 2b10 336ms

I _think_ this is the wrong place to insert these. 2b00 84ms refers to
YTPHY_WCR_PULSE_WIDTH_MASK which is used with YTPHY_WOL_CONFIG_REG? So
you insert these in the middle of WOL bits.

The sorting of registers is a bit messed up in this driver, but i
think you need to insert before:

#define YTPHY_PAD_DRIVE_STRENGTH_REG		0xA010

    Andrew

---
pw-bot: cr

Re: [PATCH 1/2] net: phy: motorcomm: add led_brightness_set for YT8521

[Chukun Pan]

Hi,

> Has this been tested on real hardware?

I tested yt8531 on some SBCs and will add this to the commit message.

> Have you had it blinking the heartbeat? Shift-lock etc.

Only supports forced LED on/off. Forced blink is disabled by default.
Do I need to implement heartbeat?

> I _think_ this is the wrong place to insert these.
>
> The sorting of registers is a bit messed up in this driver, but i
> think you need to insert before:
>
> #define YTPHY_PAD_DRIVE_STRENGTH_REG		0xA010

Indeed, I will correct this.

Thanks,
Chukun

Re: [PATCH 1/2] net: phy: motorcomm: add led_brightness_set for YT8521

[Andrew Lunn]

> Only supports forced LED on/off. Forced blink is disabled by default.
> Do I need to implement heartbeat?

drivers/leds/trigger/ledtrig-heartbeat.c

You can enable it just like any other trigger.

I find it useful to know when my bugs have caused a deadlock, etc.

    Andrew

Re: [PATCH 1/2] net: phy: motorcomm: add led_brightness_set for YT8521

[Chukun Pan]

Hi,

> You can enable it just like any other trigger.
>
> I find it useful to know when my bugs have caused a deadlock, etc.

Okay, I understand. I tested the heartbeat mode and no issues occurred.

mdio-bus:01:amber:lan# echo heartbeat > trigger
mdio-bus:01:green:lan# echo heartbeat > trigger

Thanks,
Chukun

Re: [PATCH 1/2] net: phy: motorcomm: add led_brightness_set for YT8521

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] yt8521_led_brightness_set()
  (drivers/net/phy/motorcomm.c:1782-1805) runs three indirect extended-…
- [Medium] When value != LED_OFF, the new yt8521_led_brightness_set()
  sets YTPHY_LED_FORCE_EN(index)|YTPHY_LED_FORCE_MODE(index) in…
- [Medium] In yt8521_led_brightness_set() (motorcomm.c:1796), the int
  returned by ytphy_read_ext() goes into `u16 regs` with no `< 0` check.
- [Low] The new macros YTPHY_LED_FORCE_MODE(x)=BIT(x*3) and
  YTPHY_LED_FORCE_EN(x)=BIT(x*3+2) (motorcomm.c:261-263) use a 3-bit…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the existing
  yt8521_led_hw_control_set() and yt8521_led_hw_control_get() (added in…

--

net: phy: motorcomm: add led_brightness_set for YT8521

This commit adds a led_brightness_set callback for the YT8521 and YT8531
PHYs. The callback zeroes the per-LED hardware config register. It then
sets or clears the per-LED force enable/mode bits in the LED general
config register (0xA00B).

> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index 463e1ea3efc93..bcd3afef0d496 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c
> @@ -258,6 +258,10 @@
>  #define YTPHY_WCR_INTR_SEL			BIT(6)
>  #define YTPHY_WCR_ENABLE			BIT(3)
>  
> +#define YTPHY_LED_GENERAL_CFG_REG		0xA00B
> +#define YTPHY_LED_FORCE_MODE(x)			BIT((x) * 3)
> +#define YTPHY_LED_FORCE_EN(x)			BIT((x) * 3 + 2)

[Severity: Low]
These macros use a 3 bit stride per LED, but only two of the three bits
have names. Is bit (x) * 3 + 1 the upper half of a 2 bit force mode
field?

If it is, yt8521_led_brightness_set() only sets or clears BIT((x) * 3)
and leaves the middle bit as it was. Nothing else in the driver
initializes 0xA00B. If a bootloader left that bit set, would turning
the LED on program force mode 0b11 instead of 0b01?

Would it be better to define the field with GENMASK() and write the
full "force on" encoding? I couldn't check this against the
YT8521/YT8531 datasheet, so this is based only on the layout the macros
imply.

> +
>  /* 2b00 84ms
>   * 2b01 168ms  *default*
>   * 2b10 336ms

[ ... ]

> @@ -1775,6 +1779,31 @@ static int yt8531s_config_init(struct phy_device *phydev)
>  	return phy_restore_page(phydev, old_page, ret);
>  }
>  
> +static int yt8521_led_brightness_set(struct phy_device *phydev, u8 index,
> +				     enum led_brightness value)
> +{
> +	u16 regs;
> +	int ret;
> +
> +	if (index >= YT8521_MAX_LEDS)
> +		return -EINVAL;
> +
> +	/* clear HW LED setup */
> +	ret = ytphy_write_ext(phydev, YT8521_LED0_CFG_REG + index, 0);

[Severity: High]
Is the MDIO bus lock held here? The kernel-doc for ytphy_read_ext() and
ytphy_write_ext() says:

    NOTE:The caller must have taken the MDIO bus lock.

The only caller is phy_led_set_brightness() in
drivers/net/phy/phy_device.c, and it only takes phydev->lock:

    mutex_lock(&phydev->lock);
    err = phydev->drv->led_brightness_set(phydev, phyled->index, value);
    mutex_unlock(&phydev->lock);

That means all three extended register accesses in this function reach
__mdiobus_write() and __mdiobus_read() without bus->mdio_lock. With
lockdep enabled, wouldn't this trip
lockdep_assert_held_once(&bus->mdio_lock)?

Without lockdep, the MDIO controller can be entered at the same time for
another PHY on the same bus. Another mdio_lock-only user of this PHY can
also change YTPHY_PAGE_SELECT between our page select write and the
YTPHY_PAGE_DATA access.

If that happens, the 0 written to LEDx_CFG, or the read-modify-write of
0xA00B, lands on whatever extended register the other context selected,
for example the 0xA000 UTP/fiber space select.

Software triggers reach this path repeatedly from set_brightness_work.
Examples are timer, heartbeat, and netdev when offload isn't possible.

Could the whole sequence run inside a single phy_lock_mdio_bus() /
phy_unlock_mdio_bus() section? The driver already has the *_with_lock
helpers.

The later patch "net: phy: motorcomm: add missing trigger link for
YT8521" only changes yt8521_led_hw_control_get(), so this is still
present at the end of the series.

> +	if (ret < 0)
> +		return ret;
> +
> +	regs = ytphy_read_ext(phydev, YTPHY_LED_GENERAL_CFG_REG);

[Severity: Medium]
What happens when ytphy_read_ext() fails here? It returns a negative
errno, but regs is a u16 and the result is never checked for an error.

-EIO truncates to 0xFFFB, so this LED's bits are ORed in or cleared and
the value is written back to YTPHY_LED_GENERAL_CFG_REG. That forces the
other two LEDs and sets any other bits that 0xA00B holds.

LEDx_CFG has already been zeroed above. If the final write succeeds, the
caller gets 0 back.

yt8521_led_hw_control_get() in the same file uses an int and checks
val < 0. Could ytphy_modify_ext() or ytphy_modify_ext_with_lock() do
this read-modify-write instead, since they propagate the read error?

> +	if (value != LED_OFF)
> +		regs |= YTPHY_LED_FORCE_EN(index) |
> +			YTPHY_LED_FORCE_MODE(index);

[Severity: Medium]
After these force bits are set, what clears them when the LED goes back
to hardware control? yt8521_led_hw_control_set() only writes
YT8521_LED0_CFG_REG + index and never touches 0xA00B.

The LED core doesn't always send LED_OFF before hardware control takes
over. In __led_trigger_set(), the LED_OFF call sits inside
if (led_cdev->trigger), so this sequence skips it:

    echo none > trigger
    echo 1 > brightness
    echo netdev > trigger

set_baseline_state() in ledtrig-netdev.c then calls
led_cdev->hw_control_set() directly. The rules go into LEDx_CFG, but
FORCE_EN is still set in 0xA00B. Wouldn't the LED then stay on whatever
the link or activity state?

yt8521_led_hw_control_get() also reads only LEDx_CFG. In force mode it
would return 0 with no rules set. The led_hw_control_get kernel-doc in
include/linux/phy.h asks for an error code "if the current blinking
cannot be represented in rules".

Should yt8521_led_hw_control_set() clear FORCE_EN and FORCE_MODE for
the index? Should yt8521_led_hw_control_get() account for force mode?

The later patch "net: phy: motorcomm: add missing trigger link for
YT8521" doesn't touch the force bits either.

> +	else
> +		regs &= ~(YTPHY_LED_FORCE_EN(index) |
> +			  YTPHY_LED_FORCE_MODE(index));
> +
> +	return ytphy_write_ext(phydev, YTPHY_LED_GENERAL_CFG_REG, regs);
> +}
> +

[ ... ]

> @@ -3266,6 +3295,7 @@ static struct phy_driver motorcomm_phy_drvs[] = {
>  		.soft_reset	= yt8521_soft_reset,
>  		.suspend	= yt8521_suspend,
>  		.resume		= yt8521_resume,
> +		.led_brightness_set = yt8521_led_brightness_set,
>  		.led_hw_is_supported = yt8521_led_hw_is_supported,
>  		.led_hw_control_set = yt8521_led_hw_control_set,
>  		.led_hw_control_get = yt8521_led_hw_control_get,

[Severity: High]
This is a pre-existing issue and wasn't introduced by this patch.
yt8521_led_hw_control_set() and yt8521_led_hw_control_get() are also
missing the MDIO bus lock.

phy_led_hw_control_set() and phy_led_hw_control_get() in
drivers/net/phy/phy_device.c only hold phydev->lock around the
callbacks, and both callbacks use the unlocked helpers:

yt8521_led_hw_control_set():
    return ytphy_write_ext(phydev, YT8521_LED0_CFG_REG + index, val);

yt8521_led_hw_control_get():
    val = ytphy_read_ext(phydev, YT8521_LED0_CFG_REG + index);

Both came in with commit 355b82c54c12 ("net: phy: motorcomm: Add
support for PHY LEDs on YT8521"), and the new brightness callback follows
the same pattern.

Could the locking fix cover all three LED callbacks, for example with
phy_lock_mdio_bus() or the *_with_lock helpers?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110100001.1873309-1-amadeus%40jmu.edu.cn