[PATCH net-next] net: phy: mscc: Add support for PHY LEDs on VSC8541

[PATCH net-next] net: phy: mscc: Add support for PHY LEDs on VSC8541

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add a minimal LED controller implementation supporting common use cases
with the 'netdev' trigger.

The driver now defaults to VSC8531_LINK_ACTIVITY at initialization and
allows users to configure LED behavior through the LED subsystem. Support
for controlling LED behavior is also added.

The LED Behavior (register 30) bits [0:1] control the combine feature:
0: Combine enabled (link/activity, duplex/collision)
1: Combine disabled (link only, duplex only)

This feature is now managed based on the RX/TX rules. If both RX and TX
are disabled, the combine feature is turned off; otherwise, it remains
enabled.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/phy/mscc/mscc.h      |   4 +
 drivers/net/phy/mscc/mscc_main.c | 223 ++++++++++++++++++++++++++++++-
 2 files changed, 222 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index 2eef5956b9cc..65c9d7bd9315 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -85,6 +85,10 @@ enum rgmii_clock_delay {
 #define LED_MODE_SEL_MASK(x)		  (GENMASK(3, 0) << LED_MODE_SEL_POS(x))
 #define LED_MODE_SEL(x, mode)		  (((mode) << LED_MODE_SEL_POS(x)) & LED_MODE_SEL_MASK(x))
 
+#define MSCC_PHY_LED_BEHAVIOR		  30
+#define LED_COMBINE_DIS_MASK(x)		  BIT(x)
+#define LED_COMBINE_DIS(x, dis)		  (((dis) ? 1 : 0) << (x))
+
 #define MSCC_EXT_PAGE_CSR_CNTL_17	  17
 #define MSCC_EXT_PAGE_CSR_CNTL_18	  18
 
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 8678ebf89cca..0c4e368527b5 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -173,23 +173,43 @@ static void vsc85xx_get_stats(struct phy_device *phydev,
 		data[i] = vsc85xx_get_stat(phydev, i);
 }
 
-static int vsc85xx_led_cntl_set(struct phy_device *phydev,
-				u8 led_num,
-				u8 mode)
+static int vsc85xx_led_cntl_set_lock_unlock(struct phy_device *phydev,
+					    u8 led_num,
+					    u8 mode, bool lock)
 {
 	int rc;
 	u16 reg_val;
 
-	mutex_lock(&phydev->lock);
+	if (lock)
+		mutex_lock(&phydev->lock);
 	reg_val = phy_read(phydev, MSCC_PHY_LED_MODE_SEL);
 	reg_val &= ~LED_MODE_SEL_MASK(led_num);
 	reg_val |= LED_MODE_SEL(led_num, (u16)mode);
 	rc = phy_write(phydev, MSCC_PHY_LED_MODE_SEL, reg_val);
-	mutex_unlock(&phydev->lock);
+	if (lock)
+		mutex_unlock(&phydev->lock);
 
 	return rc;
 }
 
+static int vsc85xx_led_cntl_set(struct phy_device *phydev, u8 led_num,
+				u8 mode)
+{
+	return vsc85xx_led_cntl_set_lock_unlock(phydev, led_num, mode, true);
+}
+
+static int vsc8541_led_combine_disable_set(struct phy_device *phydev, u8 led_num,
+					   bool combine_disable)
+{
+	u16 reg_val;
+
+	reg_val = phy_read(phydev, MSCC_PHY_LED_BEHAVIOR);
+	reg_val &= ~LED_COMBINE_DIS_MASK(led_num);
+	reg_val |= LED_COMBINE_DIS(led_num, combine_disable);
+
+	return phy_write(phydev, MSCC_PHY_LED_BEHAVIOR, reg_val);
+}
+
 static int vsc85xx_mdix_get(struct phy_device *phydev, u8 *mdix)
 {
 	u16 reg_val;
@@ -2218,6 +2238,174 @@ static int vsc85xx_config_inband(struct phy_device *phydev, unsigned int modes)
 				reg_val);
 }
 
+static int vsc8541_led_brightness_set(struct phy_device *phydev,
+				      u8 index, enum led_brightness value)
+{
+	struct vsc8531_private *vsc8531 = phydev->priv;
+
+	if (index >= vsc8531->nleds)
+		return -EINVAL;
+
+	return vsc85xx_led_cntl_set_lock_unlock(phydev, index, value == LED_OFF ?
+				    VSC8531_FORCE_LED_OFF : VSC8531_FORCE_LED_ON, false);
+}
+
+static int vsc8541_led_hw_is_supported(struct phy_device *phydev, u8 index,
+				       unsigned long rules)
+{
+	struct vsc8531_private *vsc8531 = phydev->priv;
+	static const unsigned long supported = BIT(TRIGGER_NETDEV_LINK) |
+					       BIT(TRIGGER_NETDEV_LINK_1000) |
+					       BIT(TRIGGER_NETDEV_LINK_100) |
+					       BIT(TRIGGER_NETDEV_LINK_10) |
+					       BIT(TRIGGER_NETDEV_RX) |
+					       BIT(TRIGGER_NETDEV_TX);
+
+	if (index >= vsc8531->nleds)
+		return -EINVAL;
+
+	if (rules & ~supported)
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
+static int vsc8541_led_hw_control_get(struct phy_device *phydev, u8 index,
+				      unsigned long *rules)
+{
+	struct vsc8531_private *vsc8531 = phydev->priv;
+	u16 reg;
+
+	if (index >= vsc8531->nleds)
+		return -EINVAL;
+
+	reg = phy_read(phydev, MSCC_PHY_LED_MODE_SEL) & LED_MODE_SEL_MASK(index);
+	reg >>= LED_MODE_SEL_POS(index);
+	switch (reg) {
+	case VSC8531_LINK_ACTIVITY:
+		*rules = BIT(TRIGGER_NETDEV_LINK) |
+			 BIT(TRIGGER_NETDEV_RX) |
+			 BIT(TRIGGER_NETDEV_TX);
+		break;
+
+	case VSC8531_LINK_1000_ACTIVITY:
+		*rules = BIT(TRIGGER_NETDEV_LINK) |
+			 BIT(TRIGGER_NETDEV_LINK_1000) |
+			 BIT(TRIGGER_NETDEV_RX) |
+			 BIT(TRIGGER_NETDEV_TX);
+		break;
+
+	case VSC8531_LINK_100_ACTIVITY:
+		*rules = BIT(TRIGGER_NETDEV_LINK) |
+			 BIT(TRIGGER_NETDEV_LINK_100) |
+			 BIT(TRIGGER_NETDEV_RX) |
+			 BIT(TRIGGER_NETDEV_TX);
+		break;
+
+	case VSC8531_LINK_10_ACTIVITY:
+		*rules = BIT(TRIGGER_NETDEV_LINK) |
+			 BIT(TRIGGER_NETDEV_LINK_10) |
+			 BIT(TRIGGER_NETDEV_RX) |
+			 BIT(TRIGGER_NETDEV_TX);
+		break;
+
+	case VSC8531_LINK_100_1000_ACTIVITY:
+		*rules = BIT(TRIGGER_NETDEV_LINK) |
+			 BIT(TRIGGER_NETDEV_LINK_100) |
+			 BIT(TRIGGER_NETDEV_LINK_1000) |
+			 BIT(TRIGGER_NETDEV_RX) |
+			 BIT(TRIGGER_NETDEV_TX);
+		break;
+
+	case VSC8531_LINK_10_1000_ACTIVITY:
+		*rules = BIT(TRIGGER_NETDEV_LINK) |
+			 BIT(TRIGGER_NETDEV_LINK_10) |
+			 BIT(TRIGGER_NETDEV_LINK_1000) |
+			 BIT(TRIGGER_NETDEV_RX) |
+			 BIT(TRIGGER_NETDEV_TX);
+		break;
+
+	case VSC8531_LINK_10_100_ACTIVITY:
+		*rules = BIT(TRIGGER_NETDEV_LINK) |
+			 BIT(TRIGGER_NETDEV_LINK_10) |
+			 BIT(TRIGGER_NETDEV_LINK_100) |
+			 BIT(TRIGGER_NETDEV_RX) |
+			 BIT(TRIGGER_NETDEV_TX);
+		break;
+
+	case VSC8531_ACTIVITY:
+		*rules = BIT(TRIGGER_NETDEV_LINK) |
+			 BIT(TRIGGER_NETDEV_RX) |
+			 BIT(TRIGGER_NETDEV_TX);
+		break;
+
+	default:
+		*rules = 0;
+		break;
+	}
+
+	return 0;
+}
+
+static int vsc8541_led_hw_control_set(struct phy_device *phydev, u8 index,
+				      unsigned long rules)
+{
+	struct vsc8531_private *vsc8531 = phydev->priv;
+	bool combine_disable = false;
+	u16 mode = VSC8531_LINK_ACTIVITY;
+	bool has_rx, has_tx;
+	int ret;
+
+	if (index >= vsc8531->nleds)
+		return -EINVAL;
+
+	if (rules & BIT(TRIGGER_NETDEV_LINK))
+		mode = VSC8531_LINK_ACTIVITY;
+
+	if (rules & BIT(TRIGGER_NETDEV_LINK_10))
+		mode = VSC8531_LINK_10_ACTIVITY;
+
+	if (rules & BIT(TRIGGER_NETDEV_LINK_100))
+		mode = VSC8531_LINK_100_ACTIVITY;
+
+	if (rules & BIT(TRIGGER_NETDEV_LINK_1000))
+		mode = VSC8531_LINK_1000_ACTIVITY;
+
+	if (rules & BIT(TRIGGER_NETDEV_LINK_100) &&
+	    rules & BIT(TRIGGER_NETDEV_LINK_1000))
+		mode = VSC8531_LINK_100_1000_ACTIVITY;
+
+	if (rules & BIT(TRIGGER_NETDEV_LINK_10) &&
+	    rules & BIT(TRIGGER_NETDEV_LINK_1000))
+		mode = VSC8531_LINK_10_1000_ACTIVITY;
+
+	if (rules & BIT(TRIGGER_NETDEV_LINK_10) &&
+	    rules & BIT(TRIGGER_NETDEV_LINK_100))
+		mode = VSC8531_LINK_10_100_ACTIVITY;
+
+	/*
+	 * The VSC8541 PHY provides an option to control LED behavior. By
+	 * default, the LEDx combine function is enabled, meaning the LED
+	 * will be on when there is link/activity or duplex/collision. If
+	 * the combine function is disabled, the LED will be on only for
+	 * link or duplex.
+	 *
+	 * To control this behavior, we check the selected rules. If both
+	 * RX and TX activity are not selected, the LED combine function
+	 * is disabled; otherwise, it remains enabled.
+	 */
+	has_rx = !!(rules & BIT(TRIGGER_NETDEV_RX));
+	has_tx = !!(rules & BIT(TRIGGER_NETDEV_TX));
+	if (!has_rx && !has_tx)
+		combine_disable = true;
+
+	ret = vsc8541_led_combine_disable_set(phydev, index, combine_disable);
+	if (ret < 0)
+		return ret;
+
+	return vsc85xx_led_cntl_set_lock_unlock(phydev, index, mode, false);
+}
+
 static int vsc8514_probe(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531;
@@ -2322,6 +2510,7 @@ static int vsc85xx_probe(struct phy_device *phydev)
 	int rate_magic;
 	u32 default_mode[2] = {VSC8531_LINK_1000_ACTIVITY,
 	   VSC8531_LINK_100_ACTIVITY};
+	int phy_id;
 
 	rate_magic = vsc85xx_edge_rate_magic_get(phydev);
 	if (rate_magic < 0)
@@ -2343,6 +2532,26 @@ static int vsc85xx_probe(struct phy_device *phydev)
 	if (!vsc8531->stats)
 		return -ENOMEM;
 
+	phy_id = phydev->drv->phy_id & phydev->drv->phy_id_mask;
+	if (phy_id == PHY_ID_VSC8541) {
+		struct device_node *np;
+
+		/*
+		 * Check for LED configuration in device tree if available
+		 * or fall back to default `vsc8531,led-x-mode` DT properties.
+		 */
+		np = of_get_child_by_name(phydev->mdio.dev.of_node, "leds");
+		if (np) {
+			of_node_put(np);
+
+			/* default to link activity */
+			for (unsigned int i = 0; i < vsc8531->nleds; i++)
+				vsc8531->leds_mode[i] = VSC8531_LINK_ACTIVITY;
+
+			return 0;
+		}
+	}
+
 	return vsc85xx_dt_led_modes_get(phydev, default_mode);
 }
 
@@ -2548,6 +2757,10 @@ static struct phy_driver vsc85xx_driver[] = {
 	.get_sset_count = &vsc85xx_get_sset_count,
 	.get_strings    = &vsc85xx_get_strings,
 	.get_stats      = &vsc85xx_get_stats,
+	.led_brightness_set = vsc8541_led_brightness_set,
+	.led_hw_is_supported = vsc8541_led_hw_is_supported,
+	.led_hw_control_get = vsc8541_led_hw_control_get,
+	.led_hw_control_set = vsc8541_led_hw_control_set,
 },
 {
 	.phy_id		= PHY_ID_VSC8552,
-- 
2.43.0

Re: [PATCH net-next] net: phy: mscc: Add support for PHY LEDs on VSC8541

[Andrew Lunn]

> +static int vsc85xx_led_cntl_set_lock_unlock(struct phy_device *phydev,
> +					    u8 led_num,
> +					    u8 mode, bool lock)
>  {
>  	int rc;
>  	u16 reg_val;
>  
> -	mutex_lock(&phydev->lock);
> +	if (lock)
> +		mutex_lock(&phydev->lock);
>  	reg_val = phy_read(phydev, MSCC_PHY_LED_MODE_SEL);
>  	reg_val &= ~LED_MODE_SEL_MASK(led_num);
>  	reg_val |= LED_MODE_SEL(led_num, (u16)mode);
>  	rc = phy_write(phydev, MSCC_PHY_LED_MODE_SEL, reg_val);
> -	mutex_unlock(&phydev->lock);
> +	if (lock)
> +		mutex_unlock(&phydev->lock);
>  
>  	return rc;
>  }

The normal way to do this is have _vsc85xx_led_cntl_set() manipulate
the hardware, no locking. And have vsc85xx_led_cntl_set() take the
lock, call _vsc85xx_led_cntl_set(), and then release the lock. You can
then call _vsc85xx_led_cntl_set() if needed.

> +static int vsc8541_led_combine_disable_set(struct phy_device *phydev, u8 led_num,
> +					   bool combine_disable)
> +{
> +	u16 reg_val;
> +
> +	reg_val = phy_read(phydev, MSCC_PHY_LED_BEHAVIOR);

phy_read() can return a negative value. You should not assign that to
a u16.

Also, BEHAVIOUR.

> +	reg_val &= ~LED_COMBINE_DIS_MASK(led_num);
> +	reg_val |= LED_COMBINE_DIS(led_num, combine_disable);
> +
> +	return phy_write(phydev, MSCC_PHY_LED_BEHAVIOR, reg_val);

You can probably use phy_modify() here.

> +static int vsc8541_led_hw_is_supported(struct phy_device *phydev, u8 index,
> +				       unsigned long rules)
> +{
> +	struct vsc8531_private *vsc8531 = phydev->priv;
> +	static const unsigned long supported = BIT(TRIGGER_NETDEV_LINK) |
> +					       BIT(TRIGGER_NETDEV_LINK_1000) |
> +					       BIT(TRIGGER_NETDEV_LINK_100) |
> +					       BIT(TRIGGER_NETDEV_LINK_10) |
> +					       BIT(TRIGGER_NETDEV_RX) |
> +					       BIT(TRIGGER_NETDEV_TX);
> +

Reverse Christmas tree. The lines should be sorted, longest first,
shortest last.

> +static int vsc8541_led_hw_control_get(struct phy_device *phydev, u8 index,
> +				      unsigned long *rules)
> +{
> +	struct vsc8531_private *vsc8531 = phydev->priv;
> +	u16 reg;
> +
> +	if (index >= vsc8531->nleds)
> +		return -EINVAL;
> +
> +	reg = phy_read(phydev, MSCC_PHY_LED_MODE_SEL) & LED_MODE_SEL_MASK(index);

Another cause of u16, when int should be used. Please check all
instances of phy_read().

> +	reg >>= LED_MODE_SEL_POS(index);
> +	switch (reg) {
> +	case VSC8531_LINK_ACTIVITY:
> +		*rules = BIT(TRIGGER_NETDEV_LINK) |
> +			 BIT(TRIGGER_NETDEV_RX) |
> +			 BIT(TRIGGER_NETDEV_TX);
> +		break;
> +
> +	case VSC8531_LINK_1000_ACTIVITY:
> +		*rules = BIT(TRIGGER_NETDEV_LINK) |
> +			 BIT(TRIGGER_NETDEV_LINK_1000) |
> +			 BIT(TRIGGER_NETDEV_RX) |
> +			 BIT(TRIGGER_NETDEV_TX);
> +		break;
> +
> +	case VSC8531_LINK_100_ACTIVITY:
> +		*rules = BIT(TRIGGER_NETDEV_LINK) |
> +			 BIT(TRIGGER_NETDEV_LINK_100) |
> +			 BIT(TRIGGER_NETDEV_RX) |
> +			 BIT(TRIGGER_NETDEV_TX);
> +		break;
> +
> +	case VSC8531_LINK_10_ACTIVITY:
> +		*rules = BIT(TRIGGER_NETDEV_LINK) |
> +			 BIT(TRIGGER_NETDEV_LINK_10) |
> +			 BIT(TRIGGER_NETDEV_RX) |
> +			 BIT(TRIGGER_NETDEV_TX);
> +		break;
> +
> +	case VSC8531_LINK_100_1000_ACTIVITY:
> +		*rules = BIT(TRIGGER_NETDEV_LINK) |
> +			 BIT(TRIGGER_NETDEV_LINK_100) |
> +			 BIT(TRIGGER_NETDEV_LINK_1000) |
> +			 BIT(TRIGGER_NETDEV_RX) |
> +			 BIT(TRIGGER_NETDEV_TX);
> +		break;
> +
> +	case VSC8531_LINK_10_1000_ACTIVITY:
> +		*rules = BIT(TRIGGER_NETDEV_LINK) |
> +			 BIT(TRIGGER_NETDEV_LINK_10) |
> +			 BIT(TRIGGER_NETDEV_LINK_1000) |
> +			 BIT(TRIGGER_NETDEV_RX) |
> +			 BIT(TRIGGER_NETDEV_TX);
> +		break;
> +
> +	case VSC8531_LINK_10_100_ACTIVITY:
> +		*rules = BIT(TRIGGER_NETDEV_LINK) |
> +			 BIT(TRIGGER_NETDEV_LINK_10) |
> +			 BIT(TRIGGER_NETDEV_LINK_100) |
> +			 BIT(TRIGGER_NETDEV_RX) |
> +			 BIT(TRIGGER_NETDEV_TX);
> +		break;
> +
> +	case VSC8531_ACTIVITY:
> +		*rules = BIT(TRIGGER_NETDEV_LINK) |
> +			 BIT(TRIGGER_NETDEV_RX) |
> +			 BIT(TRIGGER_NETDEV_TX);
> +		break;

Should the combine bit be taken into account here?

> @@ -2343,6 +2532,26 @@ static int vsc85xx_probe(struct phy_device *phydev)
>  	if (!vsc8531->stats)
>  		return -ENOMEM;
>  
> +	phy_id = phydev->drv->phy_id & phydev->drv->phy_id_mask;
> +	if (phy_id == PHY_ID_VSC8541) {

The VSC8541 has its own probe function, vsc8514_probe(). Why is this
needed?

    Andrew

---
pw-bot: cr

Re: [PATCH net-next] net: phy: mscc: Add support for PHY LEDs on VSC8541

[Lad, Prabhakar]

Hi Andrew,

Thank you for the review.

On Thu, Nov 6, 2025 at 8:45 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > +static int vsc85xx_led_cntl_set_lock_unlock(struct phy_device *phydev,
> > +                                         u8 led_num,
> > +                                         u8 mode, bool lock)
> >  {
> >       int rc;
> >       u16 reg_val;
> >
> > -     mutex_lock(&phydev->lock);
> > +     if (lock)
> > +             mutex_lock(&phydev->lock);
> >       reg_val = phy_read(phydev, MSCC_PHY_LED_MODE_SEL);
> >       reg_val &= ~LED_MODE_SEL_MASK(led_num);
> >       reg_val |= LED_MODE_SEL(led_num, (u16)mode);
> >       rc = phy_write(phydev, MSCC_PHY_LED_MODE_SEL, reg_val);
> > -     mutex_unlock(&phydev->lock);
> > +     if (lock)
> > +             mutex_unlock(&phydev->lock);
> >
> >       return rc;
> >  }
>
> The normal way to do this is have _vsc85xx_led_cntl_set() manipulate
> the hardware, no locking. And have vsc85xx_led_cntl_set() take the
> lock, call _vsc85xx_led_cntl_set(), and then release the lock. You can
> then call _vsc85xx_led_cntl_set() if needed.
>
Ok, I will add _vsc85xx_led_cntl_set() helper and use it in
vsc85xx_led_cntl_set() and led functions (for example
vsc85xx_mdix_get).

> > +static int vsc8541_led_combine_disable_set(struct phy_device *phydev, u8 led_num,
> > +                                        bool combine_disable)
> > +{
> > +     u16 reg_val;
> > +
> > +     reg_val = phy_read(phydev, MSCC_PHY_LED_BEHAVIOR);
>
> phy_read() can return a negative value. You should not assign that to
> a u16.
>
Agreed, I will check the return value of this. I followed the approach
which was currently used in the driver.

> Also, BEHAVIOUR.
>
> > +     reg_val &= ~LED_COMBINE_DIS_MASK(led_num);
> > +     reg_val |= LED_COMBINE_DIS(led_num, combine_disable);
> > +
> > +     return phy_write(phydev, MSCC_PHY_LED_BEHAVIOR, reg_val);
>
> You can probably use phy_modify() here.
>
Agreed, that will simplify the code.

> > +static int vsc8541_led_hw_is_supported(struct phy_device *phydev, u8 index,
> > +                                    unsigned long rules)
> > +{
> > +     struct vsc8531_private *vsc8531 = phydev->priv;
> > +     static const unsigned long supported = BIT(TRIGGER_NETDEV_LINK) |
> > +                                            BIT(TRIGGER_NETDEV_LINK_1000) |
> > +                                            BIT(TRIGGER_NETDEV_LINK_100) |
> > +                                            BIT(TRIGGER_NETDEV_LINK_10) |
> > +                                            BIT(TRIGGER_NETDEV_RX) |
> > +                                            BIT(TRIGGER_NETDEV_TX);
> > +
>
> Reverse Christmas tree. The lines should be sorted, longest first,
> shortest last.
>
Agreed.

> > +static int vsc8541_led_hw_control_get(struct phy_device *phydev, u8 index,
> > +                                   unsigned long *rules)
> > +{
> > +     struct vsc8531_private *vsc8531 = phydev->priv;
> > +     u16 reg;
> > +
> > +     if (index >= vsc8531->nleds)
> > +             return -EINVAL;
> > +
> > +     reg = phy_read(phydev, MSCC_PHY_LED_MODE_SEL) & LED_MODE_SEL_MASK(index);
>
> Another cause of u16, when int should be used. Please check all
> instances of phy_read().
>
Ok.

> > +     reg >>= LED_MODE_SEL_POS(index);
> > +     switch (reg) {
> > +     case VSC8531_LINK_ACTIVITY:
> > +             *rules = BIT(TRIGGER_NETDEV_LINK) |
> > +                      BIT(TRIGGER_NETDEV_RX) |
> > +                      BIT(TRIGGER_NETDEV_TX);
> > +             break;
> > +
> > +     case VSC8531_LINK_1000_ACTIVITY:
> > +             *rules = BIT(TRIGGER_NETDEV_LINK) |
> > +                      BIT(TRIGGER_NETDEV_LINK_1000) |
> > +                      BIT(TRIGGER_NETDEV_RX) |
> > +                      BIT(TRIGGER_NETDEV_TX);
> > +             break;
> > +
> > +     case VSC8531_LINK_100_ACTIVITY:
> > +             *rules = BIT(TRIGGER_NETDEV_LINK) |
> > +                      BIT(TRIGGER_NETDEV_LINK_100) |
> > +                      BIT(TRIGGER_NETDEV_RX) |
> > +                      BIT(TRIGGER_NETDEV_TX);
> > +             break;
> > +
> > +     case VSC8531_LINK_10_ACTIVITY:
> > +             *rules = BIT(TRIGGER_NETDEV_LINK) |
> > +                      BIT(TRIGGER_NETDEV_LINK_10) |
> > +                      BIT(TRIGGER_NETDEV_RX) |
> > +                      BIT(TRIGGER_NETDEV_TX);
> > +             break;
> > +
> > +     case VSC8531_LINK_100_1000_ACTIVITY:
> > +             *rules = BIT(TRIGGER_NETDEV_LINK) |
> > +                      BIT(TRIGGER_NETDEV_LINK_100) |
> > +                      BIT(TRIGGER_NETDEV_LINK_1000) |
> > +                      BIT(TRIGGER_NETDEV_RX) |
> > +                      BIT(TRIGGER_NETDEV_TX);
> > +             break;
> > +
> > +     case VSC8531_LINK_10_1000_ACTIVITY:
> > +             *rules = BIT(TRIGGER_NETDEV_LINK) |
> > +                      BIT(TRIGGER_NETDEV_LINK_10) |
> > +                      BIT(TRIGGER_NETDEV_LINK_1000) |
> > +                      BIT(TRIGGER_NETDEV_RX) |
> > +                      BIT(TRIGGER_NETDEV_TX);
> > +             break;
> > +
> > +     case VSC8531_LINK_10_100_ACTIVITY:
> > +             *rules = BIT(TRIGGER_NETDEV_LINK) |
> > +                      BIT(TRIGGER_NETDEV_LINK_10) |
> > +                      BIT(TRIGGER_NETDEV_LINK_100) |
> > +                      BIT(TRIGGER_NETDEV_RX) |
> > +                      BIT(TRIGGER_NETDEV_TX);
> > +             break;
> > +
> > +     case VSC8531_ACTIVITY:
> > +             *rules = BIT(TRIGGER_NETDEV_LINK) |
> > +                      BIT(TRIGGER_NETDEV_RX) |
> > +                      BIT(TRIGGER_NETDEV_TX);
> > +             break;
>
> Should the combine bit be taken into account here?
>
Agreed, I will drop setting TRIGGER_NETDEV_RX/TRIGGER_NETDEV_TX from
all the above case and set it based on the combined bit like below:

if (!behavior && *rules)
        *rules |= BIT(TRIGGER_NETDEV_RX) | BIT(TRIGGER_NETDEV_TX);



> > @@ -2343,6 +2532,26 @@ static int vsc85xx_probe(struct phy_device *phydev)
> >       if (!vsc8531->stats)
> >               return -ENOMEM;
> >
> > +     phy_id = phydev->drv->phy_id & phydev->drv->phy_id_mask;
> > +     if (phy_id == PHY_ID_VSC8541) {
>
> The VSC8541 has its own probe function, vsc8514_probe(). Why is this
> needed?
>
vsc85xx_probe() is used for other PHYs along with VSC8541 hence this
check, vsc8514_probe() is for 8514 PHY.

Cheers,
Prabhakar

Re: [PATCH net-next] net: phy: mscc: Add support for PHY LEDs on VSC8541

[Russell King (Oracle)]

On Fri, Nov 07, 2025 at 10:34:32AM +0000, Lad, Prabhakar wrote:
> On Thu, Nov 6, 2025 at 8:45 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > +static int vsc85xx_led_cntl_set_lock_unlock(struct phy_device *phydev,
> > > +                                         u8 led_num,
> > > +                                         u8 mode, bool lock)
> > >  {
> > >       int rc;
> > >       u16 reg_val;
> > >
> > > -     mutex_lock(&phydev->lock);
> > > +     if (lock)
> > > +             mutex_lock(&phydev->lock);
> > >       reg_val = phy_read(phydev, MSCC_PHY_LED_MODE_SEL);
> > >       reg_val &= ~LED_MODE_SEL_MASK(led_num);
> > >       reg_val |= LED_MODE_SEL(led_num, (u16)mode);
> > >       rc = phy_write(phydev, MSCC_PHY_LED_MODE_SEL, reg_val);
> > > -     mutex_unlock(&phydev->lock);
> > > +     if (lock)
> > > +             mutex_unlock(&phydev->lock);

If you used the provided helpers rather than open-coding a read-modify-
write, then you wouldn't even need this lock. Please use phy_modify().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: phy: mscc: Add support for PHY LEDs on VSC8541

[Lad, Prabhakar]

Hi Russell,

Thank you for the review.

On Fri, Nov 7, 2025 at 12:28 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Fri, Nov 07, 2025 at 10:34:32AM +0000, Lad, Prabhakar wrote:
> > On Thu, Nov 6, 2025 at 8:45 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > > +static int vsc85xx_led_cntl_set_lock_unlock(struct phy_device *phydev,
> > > > +                                         u8 led_num,
> > > > +                                         u8 mode, bool lock)
> > > >  {
> > > >       int rc;
> > > >       u16 reg_val;
> > > >
> > > > -     mutex_lock(&phydev->lock);
> > > > +     if (lock)
> > > > +             mutex_lock(&phydev->lock);
> > > >       reg_val = phy_read(phydev, MSCC_PHY_LED_MODE_SEL);
> > > >       reg_val &= ~LED_MODE_SEL_MASK(led_num);
> > > >       reg_val |= LED_MODE_SEL(led_num, (u16)mode);
> > > >       rc = phy_write(phydev, MSCC_PHY_LED_MODE_SEL, reg_val);
> > > > -     mutex_unlock(&phydev->lock);
> > > > +     if (lock)
> > > > +             mutex_unlock(&phydev->lock);
>
> If you used the provided helpers rather than open-coding a read-modify-
> write, then you wouldn't even need this lock. Please use phy_modify().
>
Ok, I will drop this implementation and also drop the locks from
vsc85xx_led_cntl_set() and switch to phy_modify.

Cheers,
Prabhakar

Re: [PATCH net-next] net: phy: mscc: Add support for PHY LEDs on VSC8541

[Andrew Lunn]

> > > @@ -2343,6 +2532,26 @@ static int vsc85xx_probe(struct phy_device *phydev)
> > >       if (!vsc8531->stats)
> > >               return -ENOMEM;
> > >
> > > +     phy_id = phydev->drv->phy_id & phydev->drv->phy_id_mask;
> > > +     if (phy_id == PHY_ID_VSC8541) {
> >
> > The VSC8541 has its own probe function, vsc8514_probe(). Why is this
> > needed?
> >
> vsc85xx_probe() is used for other PHYs along with VSC8541 hence this
> check, vsc8514_probe() is for 8514 PHY.

Ah, sorry. I was looking at 8514, not 8541. So yes, this is needed.

However, i think all the current probe functions could do with some
cleanup. There is a lot of repeated code. That could all be moved into
a vsc85xx_probe_common(), and then a vsc8514_probe() added, which uses
this common function to do most of the work, and then handles LEDs.

Also, is the LED handling you are adding here specific to the 8541? If
you look at the datasheets for the other devices, are any the same?

	Andrew

Re: [PATCH net-next] net: phy: mscc: Add support for PHY LEDs on VSC8541

[Lad, Prabhakar]

Hi Andrew,

On Fri, Nov 7, 2025 at 1:14 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > > @@ -2343,6 +2532,26 @@ static int vsc85xx_probe(struct phy_device *phydev)
> > > >       if (!vsc8531->stats)
> > > >               return -ENOMEM;
> > > >
> > > > +     phy_id = phydev->drv->phy_id & phydev->drv->phy_id_mask;
> > > > +     if (phy_id == PHY_ID_VSC8541) {
> > >
> > > The VSC8541 has its own probe function, vsc8514_probe(). Why is this
> > > needed?
> > >
> > vsc85xx_probe() is used for other PHYs along with VSC8541 hence this
> > check, vsc8514_probe() is for 8514 PHY.
>
> Ah, sorry. I was looking at 8514, not 8541. So yes, this is needed.
>
> However, i think all the current probe functions could do with some
> cleanup. There is a lot of repeated code. That could all be moved into
> a vsc85xx_probe_common(), and then a vsc8514_probe() added, which uses
> this common function to do most of the work, and then handles LEDs.
>
Certainly the probes can be simplified into a single function. I'll
create a patch for this.

> Also, is the LED handling you are adding here specific to the 8541? If
> you look at the datasheets for the other devices, are any the same?
>
Looking at the below datasheets the LED handlings seem to be the same.
Do you want me to add this for all the PHYs? Note I can only test this
on 8541 PHY only.

VSC8541: https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/VMDS-10496.pdf
VSC8502: https://ww1.microchip.com/downloads/en/DeviceDoc/VSC8502-03_Datasheet_60001742B.pdf
VSC8514: https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10446.pdf
VSC8501: https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/VSC8501-03_Datasheet_60001741B.pdf
VSC8504: https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/60001810A.pdf
VSC8530: https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/VMDS-10516.pdf
VSC8584: https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10455.pdf
VSC8582: https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/VMDS-10421.pdf
VSC8575: https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10457.pdf
VSC8574: https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/60001807A.pdf
VSC8572: https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/60001808A.pdf
VSC8562: https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/VMDS-10475.pdf
VSC8552: https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/60001809A.pdf

Cheers,
Prabhakar

Re: [PATCH net-next] net: phy: mscc: Add support for PHY LEDs on VSC8541

[Andrew Lunn]

> Certainly the probes can be simplified into a single function. I'll
> create a patch for this.

Please do make sure of each device having its own .probe
pointer. Don't have one probe function with lots of if/else
clauses. Put what is device specific into a device specific probe, and
what is common into helpers.

> > Also, is the LED handling you are adding here specific to the 8541? If
> > you look at the datasheets for the other devices, are any the same?
> >
> Looking at the below datasheets the LED handlings seem to be the same.

That is common. So yes, please add it to them all. It does not matter
if you can only test one device.

	Andrew

Re: [PATCH net-next] net: phy: mscc: Add support for PHY LEDs on VSC8541

[Lad, Prabhakar]

Hi Andrew,

On Fri, Nov 7, 2025 at 7:01 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Certainly the probes can be simplified into a single function. I'll
> > create a patch for this.
>
> Please do make sure of each device having its own .probe
> pointer. Don't have one probe function with lots of if/else
> clauses. Put what is device specific into a device specific probe, and
> what is common into helpers.
>
I was thinking of having a cfg struct  for common probe, something
like below and each phy would populate its config and pass it to
vsc85xx_probe_common().

struct vsc85xx_probe_config {
    const struct vsc85xx_hw_stat *hw_stats;
    u8 nleds;
    u16 supp_led_modes;
    size_t nstats;
    bool use_package;
    size_t shared_size;
    bool has_ptp;
    bool check_rate_magic;
};

static int vsc85xx_probe_common(struct phy_device *phydev,
                const struct vsc85xx_probe_config *cfg,
                const u32 *default_led_mode)
{
    struct vsc8531_private *vsc8531;
    int ret;

    /* Check rate magic if needed (only for non-package PHYs) */
    if (cfg->check_rate_magic) {
        ret = vsc85xx_edge_rate_magic_get(phydev);
        if (ret < 0)
            return ret;
    }

    vsc8531 = devm_kzalloc(&phydev->mdio.dev, sizeof(*vsc8531), GFP_KERNEL);
    if (!vsc8531)
        return -ENOMEM;

    phydev->priv = vsc8531;

    /* Store rate magic if it was checked */
    if (cfg->check_rate_magic)
        vsc8531->rate_magic = ret;

    /* Set up package if needed */
    if (cfg->use_package) {
        vsc8584_get_base_addr(phydev);
        devm_phy_package_join(&phydev->mdio.dev, phydev,
                      vsc8531->base_addr, cfg->shared_size);
    }

    /* Configure LED settings */
    vsc8531->nleds = cfg->nleds;
    vsc8531->supp_led_modes = cfg->supp_led_modes;

    /* Configure hardware stats */
    vsc8531->hw_stats = cfg->hw_stats;
    vsc8531->nstats = cfg->nstats;
    vsc8531->stats = devm_kcalloc(&phydev->mdio.dev, vsc8531->nstats,
                      sizeof(u64), GFP_KERNEL);
    if (!vsc8531->stats)
        return -ENOMEM;

    /* PTP setup for VSC8584 */
    if (cfg->has_ptp) {
        if (phy_package_probe_once(phydev)) {
            ret = vsc8584_ptp_probe_once(phydev);
            if (ret)
                return ret;
        }

        ret = vsc8584_ptp_probe(phydev);
        if (ret)
            return ret;
    }

    /* Parse LED modes from device tree */
    return vsc85xx_dt_led_modes_get(phydev, default_led_mode);
}

> > > Also, is the LED handling you are adding here specific to the 8541? If
> > > you look at the datasheets for the other devices, are any the same?
> > >
> > Looking at the below datasheets the LED handlings seem to be the same.
>
> That is common. So yes, please add it to them all. It does not matter
> if you can only test one device.
>
Ok, thanks.

Cheers,
Prabhakar