diff mbox series

[net-next,v6,06/16] net: phy: phy_device: Call into the PHY driver to set LED brightness

Message ID 20230327141031.11904-7-ansuelsmth@gmail.com
State New
Headers
Series net: Add basic LED support for switch/phy |

Commit Message

Christian Marangi March 27, 2023, 2:10 p.m. UTC 
  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>
---
 drivers/net/phy/phy_device.c | 15 ++++++++++++---
 include/linux/phy.h          | 11 +++++++++++
 2 files changed, 23 insertions(+), 3 deletions(-)

Comments

Christian Marangi April 12, 2023, 11:11 p.m. UTC | #1 
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)
Florian Fainelli April 13, 2023, 1:57 p.m. UTC | #2 
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!
Florian Fainelli April 13, 2023, 2:04 p.m. UTC | #3 
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.)?
Andrew Lunn April 13, 2023, 2:43 p.m. UTC | #4 
> 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
Andrew Lunn April 13, 2023, 2:48 p.m. UTC | #5 
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 mbox series

Patch

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)