diff mbox series

[net-next,v10,3/5] net: phy: add support for PHY LEDs polarity modes

Message ID 20240125203702.4552-4-ansuelsmth@gmail.com
State New
Headers show
Series net: phy: generic polarity + LED support for qca808x | expand

Commit Message

Christian Marangi Jan. 25, 2024, 8:36 p.m. UTC
Add support for PHY LEDs polarity modes. Some PHY require LED to be set
to active low to be turned ON. Adds support for this by declaring
active-low property in DT.

PHY driver needs to declare .led_polarity_set() to configure LED
polarity modes. Function will pass the index with the LED index and a
bitmap with all the required modes to set.

Current supported modes are:
- active-low with the flag PHY_LED_ACTIVE_LOW. LED is set to active-low
  to turn it ON.
- inactive-high-impedance with the flag PHY_LED_INACTIVE_HIGH_IMPEDANCE.
  LED is set to high impedance to turn it OFF.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
Changes v10:
- Add Review tag
Changes v9:
- Make LED probe fail if modes asked but not supported
- Fix wrong function description
Changes v5:
- Rework to LED modes bitmap
Changes v4:
- Drop for global active-low
- Rework to polarity option (for marvell10g series support)
Changes v3:
- Out of RFC
Changes v2:
- Add this patch

 drivers/net/phy/phy_device.c | 16 ++++++++++++++++
 include/linux/phy.h          | 22 ++++++++++++++++++++++
 2 files changed, 38 insertions(+)

Comments

Daniel Golle May 10, 2024, 11:14 p.m. UTC | #1
Hi Christian,
Hi Andrew,

On Thu, Jan 25, 2024 at 09:36:59PM +0100, Christian Marangi wrote:
> Add support for PHY LEDs polarity modes. Some PHY require LED to be set
> to active low to be turned ON. Adds support for this by declaring
> active-low property in DT.
> 
> PHY driver needs to declare .led_polarity_set() to configure LED
> polarity modes. Function will pass the index with the LED index and a
> bitmap with all the required modes to set.
> 
> Current supported modes are:
> - active-low with the flag PHY_LED_ACTIVE_LOW. LED is set to active-low
>   to turn it ON.
> - inactive-high-impedance with the flag PHY_LED_INACTIVE_HIGH_IMPEDANCE.
>   LED is set to high impedance to turn it OFF.

Wanting to make use of this I noticed that polarity settings are only
applied once in of_phy_led(), which is not sufficient for my use-case:

I'm writing a LED driver for Aquantia PHYs and those PHYs reset the
polarity mode every time a PHY reset is triggered.

I ended up writing the patch below, but I'm not sure if phy_init_hw
should take care of this or if the polarity modes should be stored in
memory allocated by the PHY driver and re-applied by the driver after
reset (eg. in .config_init). Kinda depends on taste and on how common
this behavior is in practise, so I thought the best is to reach out to
discuss.

Let me know what you think.

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 616bd7ba46cb..1624884fd627 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1251,6 +1251,7 @@ static int phy_poll_reset(struct phy_device *phydev)
 int phy_init_hw(struct phy_device *phydev)
 {
 	int ret = 0;
+	struct phy_led *phyled;
 
 	/* Deassert the reset signal */
 	phy_device_reset(phydev, 0);
@@ -1285,6 +1286,17 @@ int phy_init_hw(struct phy_device *phydev)
 			return ret;
 	}
 
+	if (phydev->drv->led_polarity_set) {
+		list_for_each_entry(phyled, &phydev->leds, list) {
+			if (!phyled->polarity_modes)
+				continue;
+
+			ret = phydev->drv->led_polarity_set(phydev, phyled->index, phyled->polarity_modes);
+			if (ret)
+				return ret;
+		}
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL(phy_init_hw);
@@ -3316,7 +3328,6 @@ static int of_phy_led(struct phy_device *phydev,
 	struct device *dev = &phydev->mdio.dev;
 	struct led_init_data init_data = {};
 	struct led_classdev *cdev;
-	unsigned long modes = 0;
 	struct phy_led *phyled;
 	u32 index;
 	int err;
@@ -3335,18 +3346,14 @@ static int of_phy_led(struct phy_device *phydev,
 		return -EINVAL;
 
 	if (of_property_read_bool(led, "active-low"))
-		set_bit(PHY_LED_ACTIVE_LOW, &modes);
+		set_bit(PHY_LED_ACTIVE_LOW, &phyled->polarity_modes);
 	if (of_property_read_bool(led, "inactive-high-impedance"))
-		set_bit(PHY_LED_INACTIVE_HIGH_IMPEDANCE, &modes);
+		set_bit(PHY_LED_INACTIVE_HIGH_IMPEDANCE, &phyled->polarity_modes);
 
-	if (modes) {
+	if (phyled->polarity_modes) {
 		/* Return error if asked to set polarity modes but not supported */
 		if (!phydev->drv->led_polarity_set)
 			return -EINVAL;
-
-		err = phydev->drv->led_polarity_set(phydev, index, modes);
-		if (err)
-			return err;
 	}
 
 	phyled->index = index;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3ddfe7fe781a..7c8bd72e6fee 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -888,6 +888,7 @@ struct phy_led {
 	struct list_head list;
 	struct phy_device *phydev;
 	struct led_classdev led_cdev;
+	unsigned long polarity_modes;
 	u8 index;
 };
Andrew Lunn May 10, 2024, 11:58 p.m. UTC | #2
> Wanting to make use of this I noticed that polarity settings are only
> applied once in of_phy_led(), which is not sufficient for my use-case:
> 
> I'm writing a LED driver for Aquantia PHYs and those PHYs reset the
> polarity mode every time a PHY reset is triggered.
> 
> I ended up writing the patch below, but I'm not sure if phy_init_hw
> should take care of this or if the polarity modes should be stored in
> memory allocated by the PHY driver and re-applied by the driver after
> reset (eg. in .config_init). Kinda depends on taste and on how common
> this behavior is in practise, so I thought the best is to reach out to
> discuss.

There was a similar discussion recently about WoL settings getting
lost. The conclusion about that was the PHY should keep track of WoL
setting. So i would say the same applies there. Please store it in a
local priv structure.

      Andrew
Russell King (Oracle) May 11, 2024, 9:35 a.m. UTC | #3
On Sat, May 11, 2024 at 01:58:13AM +0200, Andrew Lunn wrote:
> > Wanting to make use of this I noticed that polarity settings are only
> > applied once in of_phy_led(), which is not sufficient for my use-case:
> > 
> > I'm writing a LED driver for Aquantia PHYs and those PHYs reset the
> > polarity mode every time a PHY reset is triggered.

What sort of reset? There are hard resets that set the registers back
to default, and soft resets that don't. I think you are referring to
a hard reset, but please be clear.

> > I ended up writing the patch below, but I'm not sure if phy_init_hw
> > should take care of this or if the polarity modes should be stored in
> > memory allocated by the PHY driver and re-applied by the driver after
> > reset (eg. in .config_init). Kinda depends on taste and on how common
> > this behavior is in practise, so I thought the best is to reach out to
> > discuss.
> 
> There was a similar discussion recently about WoL settings getting
> lost. The conclusion about that was the PHY should keep track of WoL
> setting. So i would say the same applies there. Please store it in a
> local priv structure.

Agreed. If it turns out that it's something that many PHYs need to do,
that would be the tiem to move it into the core phylib code. If it only
affects a minority of drivers, then it's something drivers should do.

The reasoning here is: if its only a problem for a small amount of PHY
drivers, then we don't need to penalise everyone with additional
overhead. If it's the majority of drivers, then it makes sense to
remove this burden from drivers.

Also note that this is one of the reasons I don't particularly like
the kernel's approach to PHY hardware resets - if a platform firmware
decides to describe the PHy hardware reset to the kernel, then we end
up with the hardware reset being used at various points which will
clear all the registers back to the reset defaults including clearing
WoL settings and the like. That's fine for AN state which will get
reloaded, but other state does not, so describing the hardware reset
can make things much more complicated and introduce differing
behaviours compared to platforms that don't describe it.

A lot of platforms choose not to describe the PHY hardware reset, but
some people see that there's a way to describe it, so they do whether
or not there's a reason for the kernel to be manipulating that reset
signal.

What I'm saying is there's several issues here that all interact...
diff mbox series

Patch

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3611ea64875e..dd778c7fde1d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3097,6 +3097,7 @@  static int of_phy_led(struct phy_device *phydev,
 	struct device *dev = &phydev->mdio.dev;
 	struct led_init_data init_data = {};
 	struct led_classdev *cdev;
+	unsigned long modes = 0;
 	struct phy_led *phyled;
 	u32 index;
 	int err;
@@ -3114,6 +3115,21 @@  static int of_phy_led(struct phy_device *phydev,
 	if (index > U8_MAX)
 		return -EINVAL;
 
+	if (of_property_read_bool(led, "active-low"))
+		set_bit(PHY_LED_ACTIVE_LOW, &modes);
+	if (of_property_read_bool(led, "inactive-high-impedance"))
+		set_bit(PHY_LED_INACTIVE_HIGH_IMPEDANCE, &modes);
+
+	if (modes) {
+		/* Return error if asked to set polarity modes but not supported */
+		if (!phydev->drv->led_polarity_set)
+			return -EINVAL;
+
+		err = phydev->drv->led_polarity_set(phydev, index, modes);
+		if (err)
+			return err;
+	}
+
 	phyled->index = index;
 	if (phydev->drv->led_brightness_set)
 		cdev->brightness_set_blocking = phy_led_set_brightness;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 684efaeca07c..c9994a59ca2e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -852,6 +852,15 @@  struct phy_plca_status {
 	bool pst;
 };
 
+/* Modes for PHY LED configuration */
+enum phy_led_modes {
+	PHY_LED_ACTIVE_LOW = 0,
+	PHY_LED_INACTIVE_HIGH_IMPEDANCE = 1,
+
+	/* keep it last */
+	__PHY_LED_MODES_NUM,
+};
+
 /**
  * struct phy_led: An LED driven by the PHY
  *
@@ -1145,6 +1154,19 @@  struct phy_driver {
 	int (*led_hw_control_get)(struct phy_device *dev, u8 index,
 				  unsigned long *rules);
 
+	/**
+	 * @led_polarity_set: Set the LED polarity modes
+	 * @dev: PHY device which has the LED
+	 * @index: Which LED of the PHY device
+	 * @modes: bitmap of LED polarity modes
+	 *
+	 * Configure LED with all the required polarity modes in @modes
+	 * to make it correctly turn ON or OFF.
+	 *
+	 * Returns 0, or an error code.
+	 */
+	int (*led_polarity_set)(struct phy_device *dev, int index,
+				unsigned long modes);
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)