| Message ID | 20230619215703.4038619-3-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support offload LED blinking to PHY. | expand |
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>
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?
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?
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)
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(+)