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

[PATCH net-next v0 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 v0 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          | 14 ++++++++
 2 files changed, 82 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..1db63fb905c5 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1104,6 +1104,20 @@ 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 v0 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 v0 1/3] led: trig: netdev: Fix requesting offload device

[Andrew Lunn]

-EMORECOFFEE

Forget the cover note, which would be something like:

Extend the PHY subsystem to allow PHY drivers to offload basic LED
blinking. Provide the needed plumbing in phylib and extend the Marvell
PHY driver to offload RX activity, TX activity and link.

I will wait the usual 24 hours and then repost with a cover note.

  Andrew

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

[Russell King (Oracle)]

On Sun, Jun 18, 2023 at 07:39:37PM +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.

Hi Andrew,

What is the effect of this patch when LEDs are configured via the
existing DT property that allows arbitary register writes in this
driver?

There are a number of DTs that configure the LEDs this way, and
I don't think we would wish to regress that established configuration.

Thanks.

-- 
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 v0 3/3] net: phy: marvell: Add support for offloading LED blinking

[Andrew Lunn]

> Hi Andrew,
> 
> What is the effect of this patch when LEDs are configured via the
> existing DT property that allows arbitary register writes in this
> driver?
> 
> There are a number of DTs that configure the LEDs this way, and
> I don't think we would wish to regress that established configuration.

These patches only do anything if there is an LED subnode in the PHY
node in DT. So there should not be an changes by default.

If you have both 'marvell,reg-init' poking values into registers, and
describe the LEDs in DT, then i would expect some change in
behaviour. But no existing DT files do that.

	 Andrew

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

[Simon Horman]

On Sun, Jun 18, 2023 at 07:39:36PM +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>

...

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 11c1e91563d4..1db63fb905c5 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1104,6 +1104,20 @@ 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);
> +

Hi Andrew,

for consistency it would be nice if the comments for
the new members above was in kernel doc format.

>  };
>  #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
>  				      struct phy_driver, mdiodrv)

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

[Russell King (Oracle)]

On Mon, Jun 19, 2023 at 04:18:29PM +0200, Simon Horman wrote:
> On Sun, Jun 18, 2023 at 07:39:36PM +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>
> 
> ...
> 
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 11c1e91563d4..1db63fb905c5 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -1104,6 +1104,20 @@ 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);
> > +
> 
> Hi Andrew,
> 
> for consistency it would be nice if the comments for
> the new members above was in kernel doc format.

Unfortunately, kerneldoc doesn't understand structures-of-function-
pointers, so one can't document each operation and its parameters
without playing games such as I've done in linux/phylink.h. It involves
listing the prototypes not as function pointers but as normal function
prototypes in a #if 0..#endif section and preceeding each with a
kerneldoc comment describing the function and its parameters in the
normal way.

-- 
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 v0 2/3] net: phy: phy_device: Call into the PHY driver to set LED offload

[Simon Horman]

On Mon, Jun 19, 2023 at 07:11:04PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 19, 2023 at 04:18:29PM +0200, Simon Horman wrote:
> > On Sun, Jun 18, 2023 at 07:39:36PM +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>
> > 
> > ...
> > 
> > > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > > index 11c1e91563d4..1db63fb905c5 100644
> > > --- a/include/linux/phy.h
> > > +++ b/include/linux/phy.h
> > > @@ -1104,6 +1104,20 @@ 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);
> > > +
> > 
> > Hi Andrew,
> > 
> > for consistency it would be nice if the comments for
> > the new members above was in kernel doc format.
> 
> Unfortunately, kerneldoc doesn't understand structures-of-function-
> pointers, so one can't document each operation and its parameters
> without playing games such as I've done in linux/phylink.h. It involves
> listing the prototypes not as function pointers but as normal function
> prototypes in a #if 0..#endif section and preceeding each with a
> kerneldoc comment describing the function and its parameters in the
> normal way.

Ok. I'll confess that I wasn't aware of that problem.
But could we use one of the approaches approach already taken
for existing members of this structure?

e.g.:

        /**
         * @led_blink_set: Set a PHY LED brightness.  Index indicates
         * which of the PHYs led should be configured to blink. Delays
         * are in milliseconds and if both are zero then a sensible
         * default should be chosen.  The call should adjust the
         * timings in that case and if it can't match the values
         * specified exactly.
         */
        int (*led_blink_set)(struct phy_device *dev, u8 index,
                             unsigned long *delay_on,
                             unsigned long *delay_off);

Or the more minimalist approach:

        /** @get_plca_status: Return the current PLCA status info */
        int (*get_plca_status)(struct phy_device *dev,
                               struct phy_plca_status *plca_st);

I was going to say to be consistent. But the above aren't consistent with
each other.  I guess that I feel something is better than nothing.  But if
you think otherwise then let's let it rest.

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

[Andrew Lunn]

> Ok. I'll confess that I wasn't aware of that problem.
> But could we use one of the approaches approach already taken
> for existing members of this structure?

Yes, i will update the comments to start with /** so they look like
kerneldoc, even if they are not kerneldoc.

	Andrew