[PATCH net-next v1 0/3] Support offload LED blinking to PHY.

[PATCH net-next v1 0/3] Support offload LED blinking to PHY.

[Andrew Lunn]

Allow offloading of the LED trigger netdev to PHY drivers and
implement it for the Marvell PHY driver. Additionally, correct the
handling of when the initial state of the LED cannot be represented by
the trigger, and so an error is returned.

Since v0:

Make comments in struct phy_driver look more like kerneldoc
Add cover letter

Andrew Lunn (3):
  led: trig: netdev: Fix requesting offload device
  net: phy: phy_device: Call into the PHY driver to set LED offload
  net: phy: marvell: Add support for offloading LED blinking

 drivers/leds/trigger/ledtrig-netdev.c |   8 +-
 drivers/net/phy/marvell.c             | 243 ++++++++++++++++++++++++++
 drivers/net/phy/phy_device.c          |  68 +++++++
 include/linux/phy.h                   |  18 ++
 4 files changed, 334 insertions(+), 3 deletions(-)

-- 
2.40.1

[PATCH net-next v1 1/3] led: trig: netdev: Fix requesting offload device

[Andrew Lunn]

When the netdev trigger is activates, it tries to determine what
device the LED blinks for, and what the current blink mode is.

The documentation for hw_control_get() says:

	 * Return 0 on success, a negative error number on failing parsing the
	 * initial mode. Error from this function is NOT FATAL as the device
	 * may be in a not supported initial state by the attached LED trigger.
	 */

For the Marvell PHY and the Armada 370-rd board, the initial LED blink
mode is not supported by the trigger, so it returns an error. This
resulted in not getting the device the LED is blinking for. As a
result, the device is unknown and offloaded is never performed.

Change to condition to always get the device if offloading is
supported, and reduce the scope of testing for an error from
hw_control_get() to skip setting trigger internal state if there is an
error.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/leds/trigger/ledtrig-netdev.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 2311dae7f070..df61977f1fa2 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -471,15 +471,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	/* Check if hw control is active by default on the LED.
 	 * Init already enabled mode in hw control.
 	 */
-	if (supports_hw_control(led_cdev) &&
-	    !led_cdev->hw_control_get(led_cdev, &mode)) {
+	if (supports_hw_control(led_cdev)) {
 		dev = led_cdev->hw_control_get_device(led_cdev);
 		if (dev) {
 			const char *name = dev_name(dev);
 
 			set_device_name(trigger_data, name, strlen(name));
 			trigger_data->hw_control = true;
-			trigger_data->mode = mode;
+
+			rc = led_cdev->hw_control_get(led_cdev, &mode);
+			if (!rc)
+				trigger_data->mode = mode;
 		}
 	}
 
-- 
2.40.1

[PATCH net-next v1 2/3] net: phy: phy_device: Call into the PHY driver to set LED offload

[Andrew Lunn]

Linux LEDs can be requested to perform hardware accelerated blinking
to indicate link, RX, TX etc. Pass the rules for blinking to the PHY
driver, if it implements the ops needed to determine if a given
pattern can be offloaded, to offload it, and what the current offload
is. Additionally implement the op needed to get what device the LED is
for.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phy_device.c | 68 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          | 18 ++++++++++
 2 files changed, 86 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2cad9cc3f6b8..5397bbe418d8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3020,6 +3020,61 @@ static int phy_led_blink_set(struct led_classdev *led_cdev,
 	return err;
 }
 
+static __maybe_unused struct device *
+phy_led_hw_control_get_device(struct led_classdev *led_cdev)
+{
+	struct phy_led *phyled = to_phy_led(led_cdev);
+	struct phy_device *phydev = phyled->phydev;
+
+	if (phydev->attached_dev)
+		return &phydev->attached_dev->dev;
+	return NULL;
+}
+
+static int __maybe_unused
+phy_led_hw_control_get(struct led_classdev *led_cdev,
+		       unsigned long *rules)
+{
+	struct phy_led *phyled = to_phy_led(led_cdev);
+	struct phy_device *phydev = phyled->phydev;
+	int err;
+
+	mutex_lock(&phydev->lock);
+	err = phydev->drv->led_hw_control_get(phydev, phyled->index, rules);
+	mutex_unlock(&phydev->lock);
+
+	return err;
+}
+
+static int __maybe_unused
+phy_led_hw_control_set(struct led_classdev *led_cdev,
+		       unsigned long rules)
+{
+	struct phy_led *phyled = to_phy_led(led_cdev);
+	struct phy_device *phydev = phyled->phydev;
+	int err;
+
+	mutex_lock(&phydev->lock);
+	err = phydev->drv->led_hw_control_set(phydev, phyled->index, rules);
+	mutex_unlock(&phydev->lock);
+
+	return err;
+}
+
+static __maybe_unused int phy_led_hw_is_supported(struct led_classdev *led_cdev,
+						  unsigned long rules)
+{
+	struct phy_led *phyled = to_phy_led(led_cdev);
+	struct phy_device *phydev = phyled->phydev;
+	int err;
+
+	mutex_lock(&phydev->lock);
+	err = phydev->drv->led_hw_is_supported(phydev, phyled->index, rules);
+	mutex_unlock(&phydev->lock);
+
+	return err;
+}
+
 static int of_phy_led(struct phy_device *phydev,
 		      struct device_node *led)
 {
@@ -3048,6 +3103,19 @@ static int of_phy_led(struct phy_device *phydev,
 		cdev->brightness_set_blocking = phy_led_set_brightness;
 	if (phydev->drv->led_blink_set)
 		cdev->blink_set = phy_led_blink_set;
+
+#ifdef CONFIG_LEDS_TRIGGERS
+	if (phydev->drv->led_hw_is_supported &&
+	    phydev->drv->led_hw_control_set &&
+	    phydev->drv->led_hw_control_get) {
+		cdev->hw_control_is_supported = phy_led_hw_is_supported;
+		cdev->hw_control_set = phy_led_hw_control_set;
+		cdev->hw_control_get = phy_led_hw_control_get;
+		cdev->hw_control_trigger = "netdev";
+	}
+
+	cdev->hw_control_get_device = phy_led_hw_control_get_device;
+#endif
 	cdev->max_brightness = 1;
 	init_data.devicename = dev_name(&phydev->mdio.dev);
 	init_data.fwnode = of_fwnode_handle(led);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 11c1e91563d4..780ef2ca5ea7 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1104,6 +1104,24 @@ struct phy_driver {
 	int (*led_blink_set)(struct phy_device *dev, u8 index,
 			     unsigned long *delay_on,
 			     unsigned long *delay_off);
+	/**
+	 * Can the HW support the given rules. Return 0 if yes,
+	 * -EOPNOTSUPP if not, or an error code.
+	 */
+	int (*led_hw_is_supported)(struct phy_device *dev, u8 index,
+				   unsigned long rules);
+	/**
+	 * Set the HW to control the LED as described by rules.
+	 */
+	int (*led_hw_control_set)(struct phy_device *dev, u8 index,
+				  unsigned long rules);
+	/**
+	 * Get the rules used to describe how the HW is currently
+	 * configure.
+	 */
+	int (*led_hw_control_get)(struct phy_device *dev, u8 index,
+				  unsigned long *rules);
+
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
-- 
2.40.1

[PATCH net-next v1 3/3] net: phy: marvell: Add support for offloading LED blinking

[Andrew Lunn]

Add the code needed to indicate if a given blinking pattern can be
offloaded, to offload a pattern and to try to return the current
pattern. It is expected that ledtrig-netdev will gain support for
other patterns, such as different link speeds etc. So the code is
over-engineers to make adding such additional patterns easy.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/marvell.c | 243 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 243 insertions(+)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 43b6cb725551..a443df3034f3 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -2893,6 +2893,234 @@ static int m88e1318_led_blink_set(struct phy_device *phydev, u8 index,
 			       MII_88E1318S_PHY_LED_FUNC, reg);
 }
 
+struct marvell_led_rules {
+	int mode;
+	unsigned long rules;
+};
+
+static const struct marvell_led_rules marvell_led0[] = {
+	{
+		.mode = 0,
+		.rules = BIT(TRIGGER_NETDEV_LINK),
+	},
+	{
+		.mode = 3,
+		.rules = (BIT(TRIGGER_NETDEV_RX) |
+			  BIT(TRIGGER_NETDEV_TX)),
+	},
+	{
+		.mode = 4,
+		.rules = (BIT(TRIGGER_NETDEV_RX) |
+			  BIT(TRIGGER_NETDEV_TX)),
+	},
+	{
+		.mode = 5,
+		.rules = BIT(TRIGGER_NETDEV_TX),
+	},
+	{
+		.mode = 8,
+		.rules = 0,
+	},
+};
+
+static const struct marvell_led_rules marvell_led1[] = {
+	{
+		.mode = 1,
+		.rules = (BIT(TRIGGER_NETDEV_LINK) |
+			  BIT(TRIGGER_NETDEV_RX) |
+			  BIT(TRIGGER_NETDEV_TX)),
+	},
+	{
+		.mode = 2,
+		.rules = (BIT(TRIGGER_NETDEV_LINK) |
+			  BIT(TRIGGER_NETDEV_RX)),
+	},
+	{
+		.mode = 3,
+		.rules = (BIT(TRIGGER_NETDEV_RX) |
+			  BIT(TRIGGER_NETDEV_TX)),
+	},
+	{
+		.mode = 4,
+		.rules = (BIT(TRIGGER_NETDEV_RX) |
+			  BIT(TRIGGER_NETDEV_TX)),
+	},
+	{
+		.mode = 8,
+		.rules = 0,
+	},
+};
+
+static const struct marvell_led_rules marvell_led2[] = {
+	{
+		.mode = 0,
+		.rules = BIT(TRIGGER_NETDEV_LINK),
+	},
+	{
+		.mode = 1,
+		.rules = (BIT(TRIGGER_NETDEV_LINK) |
+			  BIT(TRIGGER_NETDEV_RX) |
+			  BIT(TRIGGER_NETDEV_TX)),
+	},
+	{
+		.mode = 3,
+		.rules = (BIT(TRIGGER_NETDEV_RX) |
+			  BIT(TRIGGER_NETDEV_TX)),
+	},
+	{
+		.mode = 4,
+		.rules = (BIT(TRIGGER_NETDEV_RX) |
+			  BIT(TRIGGER_NETDEV_TX)),
+	},
+	{
+		.mode = 5,
+		.rules = BIT(TRIGGER_NETDEV_TX),
+	},
+	{
+		.mode = 8,
+		.rules = 0,
+	},
+};
+
+static int marvell_find_led_mode(unsigned long rules,
+				 const struct marvell_led_rules *marvell_rules,
+				 int count,
+				 int *mode)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		if (marvell_rules[i].rules == rules) {
+			*mode = marvell_rules[i].mode;
+			return 0;
+		}
+	}
+	return -EOPNOTSUPP;
+}
+
+static int marvell_get_led_mode(u8 index, unsigned long rules, int *mode)
+{
+	int ret;
+
+	switch (index) {
+	case 0:
+		ret = marvell_find_led_mode(rules, marvell_led0,
+					    ARRAY_SIZE(marvell_led0), mode);
+		break;
+	case 1:
+		ret = marvell_find_led_mode(rules, marvell_led1,
+					    ARRAY_SIZE(marvell_led1), mode);
+		break;
+	case 2:
+		ret = marvell_find_led_mode(rules, marvell_led2,
+					    ARRAY_SIZE(marvell_led2), mode);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int marvell_find_led_rules(unsigned long *rules,
+				  const struct marvell_led_rules *marvell_rules,
+				  int count,
+				  int mode)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		if (marvell_rules[i].mode == mode) {
+			*rules = marvell_rules[i].rules;
+			return 0;
+		}
+	}
+	return -EOPNOTSUPP;
+}
+
+static int marvell_get_led_rules(u8 index, unsigned long *rules, int mode)
+{
+	int ret;
+
+	switch (index) {
+	case 0:
+		ret = marvell_find_led_rules(rules, marvell_led0,
+					     ARRAY_SIZE(marvell_led0), mode);
+		break;
+	case 1:
+		ret = marvell_find_led_rules(rules, marvell_led1,
+					     ARRAY_SIZE(marvell_led1), mode);
+		break;
+	case 2:
+		ret = marvell_find_led_rules(rules, marvell_led2,
+					     ARRAY_SIZE(marvell_led2), mode);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return ret;
+}
+
+static int m88e1318_led_hw_is_supported(struct phy_device *phydev, u8 index,
+					unsigned long rules)
+{
+	int mode, ret;
+
+	switch (index) {
+	case 0:
+	case 1:
+	case 2:
+		ret = marvell_get_led_mode(index, rules, &mode);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static int m88e1318_led_hw_control_set(struct phy_device *phydev, u8 index,
+				       unsigned long rules)
+{
+	int mode, ret, reg;
+
+	switch (index) {
+	case 0:
+	case 1:
+	case 2:
+		ret = marvell_get_led_mode(index, rules, &mode);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	if (ret < 0)
+		return ret;
+
+	reg = phy_read_paged(phydev, MII_MARVELL_LED_PAGE,
+			     MII_88E1318S_PHY_LED_FUNC);
+	reg &= ~(0xf << (4 * index));
+	reg |= mode << (4 * index);
+	return phy_write_paged(phydev, MII_MARVELL_LED_PAGE,
+			       MII_88E1318S_PHY_LED_FUNC, reg);
+}
+
+static int m88e1318_led_hw_control_get(struct phy_device *phydev, u8 index,
+				       unsigned long *rules)
+{
+	int mode, reg;
+
+	if (index > 2)
+		return -EINVAL;
+
+	reg = phy_read_paged(phydev, MII_MARVELL_LED_PAGE,
+			     MII_88E1318S_PHY_LED_FUNC);
+	mode = (reg >> (4 * index)) & 0xf;
+
+	return marvell_get_led_rules(index, rules, mode);
+}
+
 static int marvell_probe(struct phy_device *phydev)
 {
 	struct marvell_priv *priv;
@@ -3144,6 +3372,9 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.led_brightness_set = m88e1318_led_brightness_set,
 		.led_blink_set = m88e1318_led_blink_set,
+		.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,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1145,
@@ -3252,6 +3483,9 @@ static struct phy_driver marvell_drivers[] = {
 		.cable_test_get_status = marvell_vct7_cable_test_get_status,
 		.led_brightness_set = m88e1318_led_brightness_set,
 		.led_blink_set = m88e1318_led_blink_set,
+		.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,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1540,
@@ -3280,6 +3514,9 @@ static struct phy_driver marvell_drivers[] = {
 		.cable_test_get_status = marvell_vct7_cable_test_get_status,
 		.led_brightness_set = m88e1318_led_brightness_set,
 		.led_blink_set = m88e1318_led_blink_set,
+		.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,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1545,
@@ -3308,6 +3545,9 @@ static struct phy_driver marvell_drivers[] = {
 		.cable_test_get_status = marvell_vct7_cable_test_get_status,
 		.led_brightness_set = m88e1318_led_brightness_set,
 		.led_blink_set = m88e1318_led_blink_set,
+		.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,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E3016,
@@ -3451,6 +3691,9 @@ static struct phy_driver marvell_drivers[] = {
 		.set_tunable = m88e1540_set_tunable,
 		.led_brightness_set = m88e1318_led_brightness_set,
 		.led_blink_set = m88e1318_led_blink_set,
+		.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,
 	},
 };
 
-- 
2.40.1

Re: [PATCH net-next v1 1/3] led: trig: netdev: Fix requesting offload device

[Simon Horman]

On Mon, Jun 19, 2023 at 11:57:01PM +0200, Andrew Lunn wrote:
> When the netdev trigger is activates, it tries to determine what
> device the LED blinks for, and what the current blink mode is.
> 
> The documentation for hw_control_get() says:
> 
> 	 * Return 0 on success, a negative error number on failing parsing the
> 	 * initial mode. Error from this function is NOT FATAL as the device
> 	 * may be in a not supported initial state by the attached LED trigger.
> 	 */
> 
> For the Marvell PHY and the Armada 370-rd board, the initial LED blink
> mode is not supported by the trigger, so it returns an error. This
> resulted in not getting the device the LED is blinking for. As a
> result, the device is unknown and offloaded is never performed.
> 
> Change to condition to always get the device if offloading is
> supported, and reduce the scope of testing for an error from
> hw_control_get() to skip setting trigger internal state if there is an
> error.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index 2311dae7f070..df61977f1fa2 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -471,15 +471,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
>  	/* Check if hw control is active by default on the LED.
>  	 * Init already enabled mode in hw control.
>  	 */
> -	if (supports_hw_control(led_cdev) &&
> -	    !led_cdev->hw_control_get(led_cdev, &mode)) {
> +	if (supports_hw_control(led_cdev)) {
>  		dev = led_cdev->hw_control_get_device(led_cdev);

Hi Andrew,

Not related, and perhaps not important: I think the scope of dev could be
reduced to this block.

>  		if (dev) {
>  			const char *name = dev_name(dev);

More related, but also not important: I think the scope of mode could
be reduced tho this block.

>  
>  			set_device_name(trigger_data, name, strlen(name));
>  			trigger_data->hw_control = true;
> -			trigger_data->mode = mode;
> +
> +			rc = led_cdev->hw_control_get(led_cdev, &mode);
> +			if (!rc)
> +				trigger_data->mode = mode;

Is the case where trigger_data->hw_control is set to true
but trigger_data->mode is not set ok?

I understand that is the whole point is not to return an error in this case.
But I'm concerned about the value of trigger_data->mode.

Probably it is ok.
But I feel that it is worth asking.

>  		}
>  	}
>  
> -- 
> 2.40.1
> 

Re: [PATCH net-next v1 2/3] net: phy: phy_device: Call into the PHY driver to set LED offload

[Simon Horman]

On Mon, Jun 19, 2023 at 11:57:02PM +0200, Andrew Lunn wrote:
> Linux LEDs can be requested to perform hardware accelerated blinking
> to indicate link, RX, TX etc. Pass the rules for blinking to the PHY
> driver, if it implements the ops needed to determine if a given
> pattern can be offloaded, to offload it, and what the current offload
> is. Additionally implement the op needed to get what device the LED is
> for.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next v1 3/3] net: phy: marvell: Add support for offloading LED blinking

[Simon Horman]

On Mon, Jun 19, 2023 at 11:57:03PM +0200, Andrew Lunn wrote:
> Add the code needed to indicate if a given blinking pattern can be
> offloaded, to offload a pattern and to try to return the current
> pattern. It is expected that ledtrig-netdev will gain support for
> other patterns, such as different link speeds etc. So the code is
> over-engineers to make adding such additional patterns easy.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next v1 1/3] led: trig: netdev: Fix requesting offload device

[Andrew Lunn]

> >  			set_device_name(trigger_data, name, strlen(name));
> >  			trigger_data->hw_control = true;
> > -			trigger_data->mode = mode;
> > +
> > +			rc = led_cdev->hw_control_get(led_cdev, &mode);
> > +			if (!rc)
> > +				trigger_data->mode = mode;
> 
> Is the case where trigger_data->hw_control is set to true
> but trigger_data->mode is not set ok?
> 
> I understand that is the whole point is not to return an error in this case.
> But I'm concerned about the value of trigger_data->mode.

Yes, its something Christian and I talked about off-list.
trigger_data->mode is 0 by default due to the kzalloc(). 0 is a valid
value, it means don't blink for any reason. So in effect the LED
should be off. And any LED driver which the ledtrig-netdev.c supports
must support software control of the LED, so does support setting the
LED off.

In the normal case hw_control_get() returns indicating the current
blink mode, and the trigger sets its initial state to that. If
however, it returns an error, it probably means its current state
cannot be represented by the netdev trigger. PHY vendors do all sort
of odd things, and we don't want to support all the craziness. So
setting the LED off and leaving the user to configure the LED how they
want seems like a reasonable thing to do.

And i tested this because my initial implementation of the Marvell
driver was FUBAR and it returned an error here.

     Andrew

Re: [PATCH net-next v1 1/3] led: trig: netdev: Fix requesting offload device

[Simon Horman]

On Wed, Jun 21, 2023 at 05:19:01PM +0200, Andrew Lunn wrote:
> > >  			set_device_name(trigger_data, name, strlen(name));
> > >  			trigger_data->hw_control = true;
> > > -			trigger_data->mode = mode;
> > > +
> > > +			rc = led_cdev->hw_control_get(led_cdev, &mode);
> > > +			if (!rc)
> > > +				trigger_data->mode = mode;
> > 
> > Is the case where trigger_data->hw_control is set to true
> > but trigger_data->mode is not set ok?
> > 
> > I understand that is the whole point is not to return an error in this case.
> > But I'm concerned about the value of trigger_data->mode.
> 
> Yes, its something Christian and I talked about off-list.
> trigger_data->mode is 0 by default due to the kzalloc(). 0 is a valid
> value, it means don't blink for any reason. So in effect the LED
> should be off. And any LED driver which the ledtrig-netdev.c supports
> must support software control of the LED, so does support setting the
> LED off.
> 
> In the normal case hw_control_get() returns indicating the current
> blink mode, and the trigger sets its initial state to that. If
> however, it returns an error, it probably means its current state
> cannot be represented by the netdev trigger. PHY vendors do all sort
> of odd things, and we don't want to support all the craziness. So
> setting the LED off and leaving the user to configure the LED how they
> want seems like a reasonable thing to do.
> 
> And i tested this because my initial implementation of the Marvell
> driver was FUBAR and it returned an error here.

Thanks Andrew,

sounds good to me.
Especially,

	"we don't want to support all the craziness"

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next v1 2/3] net: phy: phy_device: Call into the PHY driver to set LED offload

[Jakub Kicinski]

On Mon, 19 Jun 2023 23:57:02 +0200 Andrew Lunn wrote:
> +	/**
> +	 * Can the HW support the given rules. Return 0 if yes,
> +	 * -EOPNOTSUPP if not, or an error code.
> +	 */
> +	int (*led_hw_is_supported)(struct phy_device *dev, u8 index,
> +				   unsigned long rules);
> +	/**
> +	 * Set the HW to control the LED as described by rules.
> +	 */
> +	int (*led_hw_control_set)(struct phy_device *dev, u8 index,
> +				  unsigned long rules);
> +	/**
> +	 * Get the rules used to describe how the HW is currently
> +	 * configure.
> +	 */
> +	int (*led_hw_control_get)(struct phy_device *dev, u8 index,
> +				  unsigned long *rules);

Why not include @led_hw_control_get in the kernel doc?
IIUC the problem is that the value doesn't get rendered when building
documentation correctly, but that should get resolved sooner or later.

OTOH what this patch adds is not valid kdoc at all, and it will never 
be valid, right?

Re: [PATCH net-next v1 2/3] net: phy: phy_device: Call into the PHY driver to set LED offload

[Jakub Kicinski]

On Wed, 21 Jun 2023 15:24:15 -0700 Jakub Kicinski wrote:
> On Mon, 19 Jun 2023 23:57:02 +0200 Andrew Lunn wrote:
> > +	/**
> > +	 * Can the HW support the given rules. Return 0 if yes,
> > +	 * -EOPNOTSUPP if not, or an error code.
> > +	 */
> > +	int (*led_hw_is_supported)(struct phy_device *dev, u8 index,
> > +				   unsigned long rules);
> > +	/**
> > +	 * Set the HW to control the LED as described by rules.
> > +	 */
> > +	int (*led_hw_control_set)(struct phy_device *dev, u8 index,
> > +				  unsigned long rules);
> > +	/**
> > +	 * Get the rules used to describe how the HW is currently
> > +	 * configure.
> > +	 */
> > +	int (*led_hw_control_get)(struct phy_device *dev, u8 index,
> > +				  unsigned long *rules);  
> 
> Why not include @led_hw_control_get in the kernel doc?
> IIUC the problem is that the value doesn't get rendered when building
> documentation correctly, but that should get resolved sooner or later.
> 
> OTOH what this patch adds is not valid kdoc at all, and it will never 
> be valid, right?

I can't repro the issue with per-member docs, BTW:

/**
 * struct abc - a grocery struct
 * @a: give me an A
 */
struct abc {
	/** @c: C is problematic? */
	void (*c)(void);
	/**
	 * @d: maybe D then?
	 */
	void (*d)(int arg, struct device *s);
	int a;
	/** @b: give me a B */
	int b;
};

/**
 * struct zabka_ops - another grocery struct
 */
struct zabka_ops {
	/** @c: C is problematic? */
	void (*c)(void);
	/**
	 * @d: maybe D then?
	 */
	void (*d)(int arg, struct device *s);
};


$ ./scripts/kernel-doc test.h

.. c:struct:: abc

  a grocery struct

.. container:: kernelindent

  **Definition**::

    struct abc {
        void (*c)(void);
        void (*d)(int arg, struct device *s);
        int a;
        int b;
    };

  **Members**

  ``c``
    C is problematic? 

  ``d``
    maybe D then?

  ``a``
    give me an A

  ``b``
    give me a B 


.. c:struct:: zabka_ops

  another grocery struct

.. container:: kernelindent

  **Definition**::

    struct zabka_ops {
        void (*c)(void);
        void (*d)(int arg, struct device *s);
    };

  **Members**

  ``c``
    C is problematic? 

  ``d``
    maybe D then?