[PATCH net-next,v2 1/2] net: phy: add LED dimming support

[PATCH net-next,v2 1/2] net: phy: add LED dimming support

[Aleksander Jan Bajkowski]

Some PHYs support LED dimming. The use case is a router that dims LEDs
at night. PHYs from different manufacturers support a different number of
brightness levels, so it was necessary to extend the API with the
led_max_brightness() function. If this function is omitted, a default
value is used, assuming that only two levels are supported.

Signed-off-by: Aleksander Jan Bajkowski <olek2@wp.pl>
Reviewed-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/phy/phy_device.c | 7 ++++++-
 include/linux/phy.h          | 7 +++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 675fbd225378..4011ececca70 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3106,7 +3106,12 @@ static int of_phy_led(struct phy_device *phydev,
 
 	cdev->hw_control_get_device = phy_led_hw_control_get_device;
 #endif
-	cdev->max_brightness = 1;
+	if (phydev->drv->led_max_brightness)
+		cdev->max_brightness =
+			phydev->drv->led_max_brightness(phydev, index);
+	else
+		cdev->max_brightness = 1;
+
 	init_data.devicename = dev_name(&phydev->mdio.dev);
 	init_data.fwnode = of_fwnode_handle(led);
 	init_data.devname_mandatory = true;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a2bfae80c449..94da2b6607a4 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1172,6 +1172,13 @@ struct phy_driver {
 	int (*led_brightness_set)(struct phy_device *dev,
 				  u8 index, enum led_brightness value);
 
+	/**
+	 * @led_max_brightness: Maximum number of brightness levels
+	 * supported by hardware. When only two levels are supported
+	 * i.e. LED_ON and LED_OFF the function can be omitted.
+	 */
+	int (*led_max_brightness)(struct phy_device *dev, u8 index);
+
 	/**
 	 * @led_blink_set: Set a PHY LED blinking.  Index indicates
 	 * which of the PHYs led should be configured to blink. Delays
-- 
2.39.5

[PATCH net-next,v2 2/2] net: phy: mxl-gpy: add LED dimming support

[Aleksander Jan Bajkowski]

Some PHYs support LED dimming. The use case is a router that dims LEDs
at night. In the GPY2xx series, the PWM control register is common to
all LEDs. To avoid confusing users, only the first LED used has
brightness control enabled. In many routers only one LED is connected
to PHY. When the minimum or maximum value is set, the PWM is turned off.

As of now, only Maxlinear PHYs support dimming. In the future, support
may be extended to other PHYs such as the Realtek RTL8221B.

Signed-off-by: Aleksander Jan Bajkowski <olek2@wp.pl>
Reviewed-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/phy/mxl-gpy.c | 100 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 95 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
index 0c8dc16ee7bd..a89a6a57bf34 100644
--- a/drivers/net/phy/mxl-gpy.c
+++ b/drivers/net/phy/mxl-gpy.c
@@ -110,9 +110,11 @@
 #define VSPEC1_MBOX_DATA	0x5
 #define VSPEC1_MBOX_ADDRLO	0x6
 #define VSPEC1_MBOX_CMD		0x7
-#define VSPEC1_MBOX_CMD_ADDRHI	GENMASK(7, 0)
-#define VSPEC1_MBOX_CMD_RD	(0 << 8)
 #define VSPEC1_MBOX_CMD_READY	BIT(15)
+#define VSPEC1_MBOX_CMD_MASK	GENMASK(11, 8)
+#define VSPEC1_MBOX_CMD_WR	0x1
+#define VSPEC1_MBOX_CMD_RD	0x0
+#define VSPEC1_MBOX_CMD_ADDRHI	GENMASK(7, 0)
 
 /* WoL */
 #define VPSPEC2_WOL_CTL		0x0E06
@@ -124,6 +126,15 @@
 /* Internal registers, access via mbox */
 #define REG_GPIO0_OUT		0xd3ce00
 
+/* LED Brightness registers, access via mbox */
+#define LED_BRT_CTRL		0xd3cf84
+#define LED_BRT_CTRL_LVLMAX	GENMASK(15, 12)
+#define LED_BRT_CTRL_LVLMIN	GENMASK(11, 8)
+#define LED_BRT_CTRL_EN		BIT(5)
+#define LED_BRT_CTRL_TSEN	BIT(4)
+
+#define GPY_MAX_BRIGHTNESS	15
+
 struct gpy_priv {
 	/* serialize mailbox acesses */
 	struct mutex mbox_lock;
@@ -138,6 +149,9 @@ struct gpy_priv {
 	 * is enabled.
 	 */
 	u64 lb_dis_to;
+
+	/* LED index with brightness control enabled */
+	int br_led_index;
 };
 
 static const struct {
@@ -261,7 +275,7 @@ static int gpy_mbox_read(struct phy_device *phydev, u32 addr)
 	if (ret)
 		goto out;
 
-	cmd = VSPEC1_MBOX_CMD_RD;
+	cmd = FIELD_PREP(VSPEC1_MBOX_CMD_MASK, VSPEC1_MBOX_CMD_RD);
 	cmd |= FIELD_PREP(VSPEC1_MBOX_CMD_ADDRHI, addr >> 16);
 
 	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_MBOX_CMD, cmd);
@@ -287,6 +301,46 @@ static int gpy_mbox_read(struct phy_device *phydev, u32 addr)
 	return ret;
 }
 
+static int gpy_mbox_write(struct phy_device *phydev, u32 addr, u16 data)
+{
+	struct gpy_priv *priv = phydev->priv;
+	int val, ret;
+	u16 cmd;
+
+	mutex_lock(&priv->mbox_lock);
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_MBOX_DATA,
+			    data);
+	if (ret)
+		goto out;
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_MBOX_ADDRLO,
+			    addr);
+	if (ret)
+		goto out;
+
+	cmd = FIELD_PREP(VSPEC1_MBOX_CMD_MASK, VSPEC1_MBOX_CMD_WR);
+	cmd |= FIELD_PREP(VSPEC1_MBOX_CMD_ADDRHI, addr >> 16);
+
+	ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_MBOX_CMD, cmd);
+	if (ret)
+		goto out;
+
+	/* The mbox read is used in the interrupt workaround. It was observed
+	 * that a read might take up to 2.5ms. This is also the time for which
+	 * the interrupt line is stuck low. To be on the safe side, poll the
+	 * ready bit for 10ms.
+	 */
+	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1,
+					VSPEC1_MBOX_CMD, val,
+					(val & VSPEC1_MBOX_CMD_READY),
+					500, 10000, false);
+
+out:
+	mutex_unlock(&priv->mbox_lock);
+	return ret;
+}
+
 static int gpy_config_init(struct phy_device *phydev)
 {
 	/* Nothing to configure. Configuration Requirement Placeholder */
@@ -317,6 +371,7 @@ static int gpy_probe(struct phy_device *phydev)
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
+	priv->br_led_index = -1;
 	phydev->priv = priv;
 	mutex_init(&priv->mbox_lock);
 
@@ -858,7 +913,8 @@ static int gpy115_loopback(struct phy_device *phydev, bool enable, int speed)
 static int gpy_led_brightness_set(struct phy_device *phydev,
 				  u8 index, enum led_brightness value)
 {
-	int ret;
+	struct gpy_priv *priv = phydev->priv;
+	int ret, reg;
 
 	if (index >= GPY_MAX_LEDS)
 		return -EINVAL;
@@ -871,7 +927,21 @@ static int gpy_led_brightness_set(struct phy_device *phydev,
 	if (ret)
 		return ret;
 
-	/* ToDo: set PWM brightness */
+	/* Set PWM brightness */
+	if (index == priv->br_led_index) {
+		if (value > LED_OFF && value < GPY_MAX_BRIGHTNESS) {
+			reg = LED_BRT_CTRL_EN;
+
+			reg |= FIELD_PREP(LED_BRT_CTRL_LVLMIN, 0x4);
+			reg |= FIELD_PREP(LED_BRT_CTRL_LVLMAX, value);
+		} else {
+			reg = LED_BRT_CTRL_TSEN;
+		}
+
+		ret = gpy_mbox_write(phydev, LED_BRT_CTRL, reg);
+		if (ret < 0)
+			return ret;
+	}
 
 	/* clear HW LED setup */
 	if (value == LED_OFF)
@@ -880,6 +950,17 @@ static int gpy_led_brightness_set(struct phy_device *phydev,
 		return 0;
 }
 
+static int gpy_led_max_brightness(struct phy_device *phydev, u8 index)
+{
+	struct gpy_priv *priv = phydev->priv;
+
+	if (priv->br_led_index < 0) {
+		priv->br_led_index = index;
+		return GPY_MAX_BRIGHTNESS;
+	}
+
+	return 1;
+}
 static const unsigned long supported_triggers = (BIT(TRIGGER_NETDEV_LINK) |
 						 BIT(TRIGGER_NETDEV_LINK_10) |
 						 BIT(TRIGGER_NETDEV_LINK_100) |
@@ -1032,6 +1113,7 @@ static struct phy_driver gpy_drivers[] = {
 		.get_wol	= gpy_get_wol,
 		.set_loopback	= gpy_loopback,
 		.led_brightness_set = gpy_led_brightness_set,
+		.led_max_brightness = gpy_led_max_brightness,
 		.led_hw_is_supported = gpy_led_hw_is_supported,
 		.led_hw_control_get = gpy_led_hw_control_get,
 		.led_hw_control_set = gpy_led_hw_control_set,
@@ -1055,6 +1137,7 @@ static struct phy_driver gpy_drivers[] = {
 		.get_wol	= gpy_get_wol,
 		.set_loopback	= gpy115_loopback,
 		.led_brightness_set = gpy_led_brightness_set,
+		.led_max_brightness = gpy_led_max_brightness,
 		.led_hw_is_supported = gpy_led_hw_is_supported,
 		.led_hw_control_get = gpy_led_hw_control_get,
 		.led_hw_control_set = gpy_led_hw_control_set,
@@ -1077,6 +1160,7 @@ static struct phy_driver gpy_drivers[] = {
 		.get_wol	= gpy_get_wol,
 		.set_loopback	= gpy115_loopback,
 		.led_brightness_set = gpy_led_brightness_set,
+		.led_max_brightness = gpy_led_max_brightness,
 		.led_hw_is_supported = gpy_led_hw_is_supported,
 		.led_hw_control_get = gpy_led_hw_control_get,
 		.led_hw_control_set = gpy_led_hw_control_set,
@@ -1100,6 +1184,7 @@ static struct phy_driver gpy_drivers[] = {
 		.get_wol	= gpy_get_wol,
 		.set_loopback	= gpy_loopback,
 		.led_brightness_set = gpy_led_brightness_set,
+		.led_max_brightness = gpy_led_max_brightness,
 		.led_hw_is_supported = gpy_led_hw_is_supported,
 		.led_hw_control_get = gpy_led_hw_control_get,
 		.led_hw_control_set = gpy_led_hw_control_set,
@@ -1122,6 +1207,7 @@ static struct phy_driver gpy_drivers[] = {
 		.get_wol	= gpy_get_wol,
 		.set_loopback	= gpy_loopback,
 		.led_brightness_set = gpy_led_brightness_set,
+		.led_max_brightness = gpy_led_max_brightness,
 		.led_hw_is_supported = gpy_led_hw_is_supported,
 		.led_hw_control_get = gpy_led_hw_control_get,
 		.led_hw_control_set = gpy_led_hw_control_set,
@@ -1145,6 +1231,7 @@ static struct phy_driver gpy_drivers[] = {
 		.get_wol	= gpy_get_wol,
 		.set_loopback	= gpy_loopback,
 		.led_brightness_set = gpy_led_brightness_set,
+		.led_max_brightness = gpy_led_max_brightness,
 		.led_hw_is_supported = gpy_led_hw_is_supported,
 		.led_hw_control_get = gpy_led_hw_control_get,
 		.led_hw_control_set = gpy_led_hw_control_set,
@@ -1167,6 +1254,7 @@ static struct phy_driver gpy_drivers[] = {
 		.get_wol	= gpy_get_wol,
 		.set_loopback	= gpy_loopback,
 		.led_brightness_set = gpy_led_brightness_set,
+		.led_max_brightness = gpy_led_max_brightness,
 		.led_hw_is_supported = gpy_led_hw_is_supported,
 		.led_hw_control_get = gpy_led_hw_control_get,
 		.led_hw_control_set = gpy_led_hw_control_set,
@@ -1190,6 +1278,7 @@ static struct phy_driver gpy_drivers[] = {
 		.get_wol	= gpy_get_wol,
 		.set_loopback	= gpy_loopback,
 		.led_brightness_set = gpy_led_brightness_set,
+		.led_max_brightness = gpy_led_max_brightness,
 		.led_hw_is_supported = gpy_led_hw_is_supported,
 		.led_hw_control_get = gpy_led_hw_control_get,
 		.led_hw_control_set = gpy_led_hw_control_set,
@@ -1212,6 +1301,7 @@ static struct phy_driver gpy_drivers[] = {
 		.get_wol	= gpy_get_wol,
 		.set_loopback	= gpy_loopback,
 		.led_brightness_set = gpy_led_brightness_set,
+		.led_max_brightness = gpy_led_max_brightness,
 		.led_hw_is_supported = gpy_led_hw_is_supported,
 		.led_hw_control_get = gpy_led_hw_control_get,
 		.led_hw_control_set = gpy_led_hw_control_set,
-- 
2.39.5

Re: [PATCH net-next,v2 1/2] net: phy: add LED dimming support

[Andrew Lunn]

On Sat, Apr 05, 2025 at 09:09:53PM +0200, Aleksander Jan Bajkowski wrote:
> Some PHYs support LED dimming. The use case is a router that dims LEDs
> at night. PHYs from different manufacturers support a different number of
> brightness levels, so it was necessary to extend the API with the
> led_max_brightness() function. If this function is omitted, a default
> value is used, assuming that only two levels are supported.
> 
> Signed-off-by: Aleksander Jan Bajkowski <olek2@wp.pl>
> Reviewed-by: Daniel Golle <daniel@makrotopia.org>

The merge window is still open at the moment, so you will need to
repost next week.


> ---
>  drivers/net/phy/phy_device.c | 7 ++++++-
>  include/linux/phy.h          | 7 +++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 675fbd225378..4011ececca70 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -3106,7 +3106,12 @@ static int of_phy_led(struct phy_device *phydev,
>  
>  	cdev->hw_control_get_device = phy_led_hw_control_get_device;
>  #endif
> -	cdev->max_brightness = 1;
> +	if (phydev->drv->led_max_brightness)
> +		cdev->max_brightness =
> +			phydev->drv->led_max_brightness(phydev, index);
> +	else
> +		cdev->max_brightness = 1;

> +	/**
> +	 * @led_max_brightness: Maximum number of brightness levels
> +	 * supported by hardware. When only two levels are supported
> +	 * i.e. LED_ON and LED_OFF the function can be omitted.
> +	 */
> +	int (*led_max_brightness)(struct phy_device *dev, u8 index);

We might want to consider types here. led_classdev->max_brightness is
an unsigned int. Your callback returns int, so it could include
-EOPNOTSUPP, -EINVAL, -ENODEV etc. There is no check for an error
code, so max_brightness is going to end up ~ 2^32, and not work very
well.

	Andrew

Re: [PATCH net-next,v2 2/2] net: phy: mxl-gpy: add LED dimming support

[Andrew Lunn]

On Sat, Apr 05, 2025 at 09:09:54PM +0200, Aleksander Jan Bajkowski wrote:
> Some PHYs support LED dimming. The use case is a router that dims LEDs
> at night. In the GPY2xx series, the PWM control register is common to
> all LEDs. To avoid confusing users, only the first LED used has
> brightness control enabled.

I don't know the LED subsystem very well. But struct led_classdev has:

        /* Get LED brightness level */
        enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);

The fact this exists, suggests the LED brightness can change outside
of the control of Linux. Maybe even your very use cases of one PWM for
multiple LEDs? You might get a more consistent user experience if you
allow the brightness bet set with all the LEDs, and implement this
callback so the current brightness can be reported per LED?

Lets see what the LED subsystem people say?

	Andrew

Re: [PATCH net-next,v2 2/2] net: phy: mxl-gpy: add LED dimming support

[Marek Behún]

On Sat, Apr 05, 2025 at 09:52:43PM +0200, Andrew Lunn wrote:
> On Sat, Apr 05, 2025 at 09:09:54PM +0200, Aleksander Jan Bajkowski wrote:
> > Some PHYs support LED dimming. The use case is a router that dims LEDs
> > at night. In the GPY2xx series, the PWM control register is common to
> > all LEDs. To avoid confusing users, only the first LED used has
> > brightness control enabled.
> 
> I don't know the LED subsystem very well. But struct led_classdev has:
> 
>         /* Get LED brightness level */
>         enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
> 
> The fact this exists, suggests the LED brightness can change outside
> of the control of Linux. Maybe even your very use cases of one PWM for
> multiple LEDs? You might get a more consistent user experience if you
> allow the brightness bet set with all the LEDs, and implement this
> callback so the current brightness can be reported per LED?
> 
> Lets see what the LED subsystem people say?

Regardless of whether LED class device blinking is offloaded to HW or
not, it should behave the same way as it does when controlled by
software.

The sysfs brightness file controls the brightness of the LED.
When no trigger is enabled on the LED, this is very simple:
- setting brightness to 0 disables the LED
- setting brightness to the value max_brightness lights the LED
  to maximum possible brightness
- values between 0 and max_brightness can be also used, to light
  the LED in some in-between value

When a trigger is set, this can get a little bit more complicated,
but not for the netdev trigger. The netdev trigger only blinks
the LED, which means that it changes it between two states:
- disabled
- brightness that was set into sysfs brightness file

So if netdev is blinking with LED, you are supposed to be able to
change the ON state brightness by writing to the sysfs brightness
file.

If you enable the netdev trigger on a LED (echo netdev >trigger)
while current brightness is set to a non-zero value, then this
non-zero value should be used for the ON state when blinking.
If you enable it while current brightness is zero, then
max_brightness is used. This is done in the set_baseline_state()
function, the blinking brightness is stored into the
led_cdev->blink_brightness member.

And also, when writing new code, or refactoring old one, stop using
the LED_OFF, LED_ON and LED_HALF and LED_FULL constants.
They are deprecated.

We should get rid of the whole enum led_brightness and use an
unsigned int instead.

Marek