| Message ID | 20230327141031.11904-7-ansuelsmth@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: Add basic LED support for switch/phy | expand |
On Thu, Apr 13, 2023 at 06:57:51AM -0700, Florian Fainelli wrote: > > > On 3/27/2023 7:10 AM, Christian Marangi wrote: > > From: Andrew Lunn <andrew@lunn.ch> > > > > Linux LEDs can be software controlled via the brightness file in /sys. > > LED drivers need to implement a brightness_set function which the core > > will call. Implement an intermediary in phy_device, which will call > > into the phy driver if it implements the necessary function. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > > + int (*led_brightness_set)(struct phy_device *dev, > > + u32 index, enum led_brightness value); > > I think I would have made this an u8, 4 billion LEDs, man, that's a lot! If andrew is ok we can still consider to reduce it. (but just to joke about it... A MAN CAN DREAM OF A FULL HD SCREEN ON THEIR OWN SPECIAL PORT)
On 3/27/2023 7:10 AM, Christian Marangi wrote: > From: Andrew Lunn <andrew@lunn.ch> > > Linux LEDs can be software controlled via the brightness file in /sys. > LED drivers need to implement a brightness_set function which the core > will call. Implement an intermediary in phy_device, which will call > into the phy driver if it implements the necessary function. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > + int (*led_brightness_set)(struct phy_device *dev, > + u32 index, enum led_brightness value); I think I would have made this an u8, 4 billion LEDs, man, that's a lot!
On 4/12/2023 4:11 PM, Christian Marangi wrote: > On Thu, Apr 13, 2023 at 06:57:51AM -0700, Florian Fainelli wrote: >> >> >> On 3/27/2023 7:10 AM, Christian Marangi wrote: >>> From: Andrew Lunn <andrew@lunn.ch> >>> >>> Linux LEDs can be software controlled via the brightness file in /sys. >>> LED drivers need to implement a brightness_set function which the core >>> will call. Implement an intermediary in phy_device, which will call >>> into the phy driver if it implements the necessary function. >>> >>> Signed-off-by: Andrew Lunn <andrew@lunn.ch> >>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> >> >> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> >> >>> + int (*led_brightness_set)(struct phy_device *dev, >>> + u32 index, enum led_brightness value); >> >> I think I would have made this an u8, 4 billion LEDs, man, that's a lot! > > If andrew is ok we can still consider to reduce it. (but just to joke > about it... A MAN CAN DREAM OF A FULL HD SCREEN ON THEIR OWN SPECIAL > PORT) Humm just thought of something else, is it OK for led_brightness_set and led_blink_set to call into functions that might require sleeping (MDIO, I2C, SPI, etc.)?
> Humm just thought of something else, is it OK for led_brightness_set and > led_blink_set to call into functions that might require sleeping (MDIO, I2C, > SPI, etc.)? Hi Florian That is fine. The LED class is similar to GPIOs. There is a can sleep version, and an atomic version. For phylib we are using the can sleep version. The LED core then hides the differences from the users. Andrew
On Thu, Apr 13, 2023 at 06:57:51AM -0700, Florian Fainelli wrote: > > > On 3/27/2023 7:10 AM, Christian Marangi wrote: > > From: Andrew Lunn <andrew@lunn.ch> > > > > Linux LEDs can be software controlled via the brightness file in /sys. > > LED drivers need to implement a brightness_set function which the core > > will call. Implement an intermediary in phy_device, which will call > > into the phy driver if it implements the necessary function. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > > + int (*led_brightness_set)(struct phy_device *dev, > > + u32 index, enum led_brightness value); > > I think I would have made this an u8, 4 billion LEDs, man, that's a lot! That can be done. We need to change: err = of_property_read_u32(led, "reg", &phyled->index); if (err) return err; to a u8, to avoid overflow problems in other places. It looks like of_property_read_u8() does the correct thing if somebody tried to use 4 billion - 1. Andrew
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 39af989947f9..141d63ef3897 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -2991,11 +2991,18 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv) return phydrv->config_intr && phydrv->handle_interrupt; } -/* Dummy implementation until calls into PHY driver are added */ static int phy_led_set_brightness(struct led_classdev *led_cdev, enum led_brightness value) { - return 0; + 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_brightness_set(phydev, phyled->index, value); + mutex_unlock(&phydev->lock); + + return err; } static int of_phy_led(struct phy_device *phydev, @@ -3012,12 +3019,14 @@ static int of_phy_led(struct phy_device *phydev, return -ENOMEM; cdev = &phyled->led_cdev; + phyled->phydev = phydev; err = of_property_read_u32(led, "reg", &phyled->index); if (err) return err; - cdev->brightness_set_blocking = phy_led_set_brightness; + if (phydev->drv->led_brightness_set) + cdev->brightness_set_blocking = phy_led_set_brightness; 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 11fb76a1c507..2a5ee66b79b0 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -841,15 +841,19 @@ struct phy_plca_status { * struct phy_led: An LED driven by the PHY * * @list: List of LEDs + * @phydev: PHY this LED is attached to * @led_cdev: Standard LED class structure * @index: Number of the LED */ struct phy_led { struct list_head list; + struct phy_device *phydev; struct led_classdev led_cdev; u32 index; }; +#define to_phy_led(d) container_of(d, struct phy_led, led_cdev) + /** * struct phy_driver - Driver structure for a particular PHY type * @@ -1072,6 +1076,13 @@ struct phy_driver { /** @get_plca_status: Return the current PLCA status info */ int (*get_plca_status)(struct phy_device *dev, struct phy_plca_status *plca_st); + + /* Set a PHY LED brightness. Index indicates which of the PHYs + * led should be set. Value follows the standard LED class meaning, + * e.g. LED_OFF, LED_HALF, LED_FULL. + */ + int (*led_brightness_set)(struct phy_device *dev, + u32 index, enum led_brightness value); }; #define to_phy_driver(d) container_of(to_mdio_common_driver(d), \ struct phy_driver, mdiodrv)