| Message ID | 20230618173937.4016322-2-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v0,1/3] led: trig: netdev: Fix requesting offload device | expand |
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)
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.
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.
> 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
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)
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(+)