[PATCH net-next] net: phy: marvell: add support for active-low and active-high LEDs

[PATCH net-next] net: phy: marvell: add support for active-low and active-high LEDs

[Aleksander Jan Bajkowski]

Add the led_polarity_set callback for setting LED polarity. Implement
this callback for the 88E1318 and 88E1510 PHYs. This should also work on
other Marvell PHYs, but I don't have access to the TRM or the hardware.

Tested on Adapteva Paralella board with Marvell 88E1318 PHY.

Signed-off-by: Aleksander Jan Bajkowski <olek2@wp.pl>
---
 drivers/net/phy/marvell.c | 43 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index f71cffa88406..432ad364efd8 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -149,6 +149,8 @@
 #define MII_88E1318S_PHY_LED_FUNC_ON		(0x9)
 #define MII_88E1318S_PHY_LED_FUNC_HI_Z		(0xa)
 #define MII_88E1318S_PHY_LED_FUNC_BLINK		(0xb)
+#define MII_88E1318S_PHY_LED_POL		0x11
+#define MII_88E1318S_PHY_LED_POL_ACT_HIGH(idx)	(1 << 2 * (idx))
 #define MII_88E1318S_PHY_LED_TCR		0x12
 #define MII_88E1318S_PHY_LED_TCR_FORCE_INT	BIT(15)
 #define MII_88E1318S_PHY_LED_TCR_INTn_ENABLE	BIT(7)
@@ -304,6 +306,8 @@
 #define NB_FIBER_STATS	1
 #define NB_STAT_MAX	3
 
+#define M88E1318S_MAX_LEDS	3
+
 MODULE_DESCRIPTION("Marvell PHY driver");
 MODULE_AUTHOR("Andy Fleming");
 MODULE_LICENSE("GPL");
@@ -3572,7 +3576,7 @@ static int m88e1318_led_hw_control_get(struct phy_device *phydev, u8 index,
 {
 	int mode, reg;
 
-	if (index > 2)
+	if (index >= M88E1318S_MAX_LEDS)
 		return -EINVAL;
 
 	reg = phy_read_paged(phydev, MII_MARVELL_LED_PAGE,
@@ -3585,6 +3589,41 @@ static int m88e1318_led_hw_control_get(struct phy_device *phydev, u8 index,
 	return marvell_get_led_rules(index, rules, mode);
 }
 
+static int m88e1318_led_polarity_set(struct phy_device *phydev, int index,
+				     unsigned long modes)
+{
+	bool force_active_low = false, force_active_high = false;
+	u32 mode;
+
+	if (index >= M88E1318S_MAX_LEDS)
+		return -EINVAL;
+
+	for_each_set_bit(mode, &modes, __PHY_LED_MODES_NUM) {
+		switch (mode) {
+		case PHY_LED_ACTIVE_LOW:
+			force_active_low = true;
+			break;
+		case PHY_LED_ACTIVE_HIGH:
+			force_active_high = true;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	if (force_active_low)
+		return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE,
+					MII_88E1318S_PHY_LED_POL,
+					MII_88E1318S_PHY_LED_POL_ACT_HIGH(index), 0);
+
+	if (force_active_high)
+		return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE,
+					MII_88E1318S_PHY_LED_POL, 0,
+					MII_88E1318S_PHY_LED_POL_ACT_HIGH(index));
+
+	return -EINVAL;
+}
+
 static int marvell_probe(struct phy_device *phydev)
 {
 	struct marvell_priv *priv;
@@ -3833,6 +3872,7 @@ static struct phy_driver marvell_drivers[] = {
 		.led_hw_is_supported = m88e1318_led_hw_is_supported,
 		.led_hw_control_set = m88e1318_led_hw_control_set,
 		.led_hw_control_get = m88e1318_led_hw_control_get,
+		.led_polarity_set = m88e1318_led_polarity_set,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1145,
@@ -3947,6 +3987,7 @@ static struct phy_driver marvell_drivers[] = {
 		.led_hw_is_supported = m88e1318_led_hw_is_supported,
 		.led_hw_control_set = m88e1318_led_hw_control_set,
 		.led_hw_control_get = m88e1318_led_hw_control_get,
+		.led_polarity_set = m88e1318_led_polarity_set,
 		.attach_mii_port = m88e1510_attach_mii_port,
 	},
 	{
-- 
2.53.0

Re: [PATCH net-next] net: phy: marvell: add support for active-low and active-high LEDs

[netdev-bot+sashiko]

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

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

- [Medium] Incomplete mask for 2-bit LED polarity field in
  phy_modify_paged.

--

> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -3585,6 +3589,41 @@ static int m88e1318_led_hw_control_get(struct phy_device *phydev, u8 index,
>  	return marvell_get_led_rules(index, rules, mode);
>  }
>  
> +static int m88e1318_led_polarity_set(struct phy_device *phydev, int index,
> +				     unsigned long modes)
> +{
> +	bool force_active_low = false, force_active_high = false;
> +	u32 mode;
> +
> +	if (index >= M88E1318S_MAX_LEDS)
> +		return -EINVAL;
> +
> +	for_each_set_bit(mode, &modes, __PHY_LED_MODES_NUM) {
> +		switch (mode) {
> +		case PHY_LED_ACTIVE_LOW:
> +			force_active_low = true;
> +			break;
> +		case PHY_LED_ACTIVE_HIGH:
> +			force_active_high = true;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (force_active_low)
> +		return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE,
> +					MII_88E1318S_PHY_LED_POL,
> +					MII_88E1318S_PHY_LED_POL_ACT_HIGH(index), 0);
> +
> +	if (force_active_high)
> +		return phy_modify_paged(phydev, MII_MARVELL_LED_PAGE,
> +					MII_88E1318S_PHY_LED_POL, 0,
> +					MII_88E1318S_PHY_LED_POL_ACT_HIGH(index));
> +
> +	return -EINVAL;
> +}
> +
>  static int marvell_probe(struct phy_device *phydev)
>  {
>  	struct marvell_priv *priv;

[Severity: Medium]
The LED polarity field in page 3 register 0x11 is two bits wide per
LED, but m88e1318_led_polarity_set() only ever touches the lower bit
of the field. MII_88E1318S_PHY_LED_POL_ACT_HIGH(idx) expands to
1 << 2 * idx, so for the active-low case the phy_modify_paged() call
clears just that one bit and leaves the upper bit of the pair
untouched; for the active-high case the mask argument is 0, so no
bits are cleared at all and only the low bit is set.

That means the result depends on whatever the field held before. If
the LED was left in one of the encodings that has the upper bit set
(e.g. the tri-state/high-Z variant configured by the bootloader or by
strapping), asking for active-low leaves the pair as 0b10 - i.e. the
requested polarity is silently not applied - and asking for
active-high produces 0b11, which is not one of the documented
encodings. Since this callback is reached from the LED polarity
plumbing on any DT/sysfs configuration, the LED can end up in a state
that neither the user nor a later led_hw_control_get() agrees with.

The fix is to read-modify-write the whole two-bit field: pass a mask
covering both bits (something like GENMASK(2 * index + 1, 2 * index),
or a dedicated MII_88E1318S_PHY_LED_POL_MASK(idx) macro) and the
desired encoding as the set value, so both the active-low and
active-high paths fully define the field. Since you have the 88E1318
hardware and I do not have the TRM in front of me, could you confirm
from the datasheet what the upper bit of each pair means and what the
correct encodings for plain active-low and active-high are? If the
upper bit selects tri-state-when-off behaviour that we deliberately
want to preserve, please say so and add a comment, but then the
active-high path still must not be able to produce the reserved 0b11
combination.

While you are there, please also use BIT()/GENMASK() and parenthesise
the shift in the new macro rather than open-coding 1 << 2 * (idx).

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914200847.445375-1-olek2%40wp.pl