| Message ID | 20240125203702.4552-1-ansuelsmth@gmail.com |
|---|---|
| Headers | show |
| Series | net: phy: generic polarity + LED support for qca808x | expand |
Hello: This series was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 25 Jan 2024 21:36:56 +0100 you wrote: > This small series add LEDs support for qca808x. > > QCA808x apply on PHY reset a strange polarity settings and require > some tweak to apply a more common configuration found on devices. > On adding support for it, it was pointed out that a similar > feature is also being implemented for a marvell PHY where > LED polarity is set per LED (and not global) and also have > a special mode where the LED is tristated. > > [...] Here is the summary with links: - [net-next,v10,1/5] dt-bindings: net: phy: Make LED active-low property common https://git.kernel.org/netdev/net-next/c/c94d1783136e - [net-next,v10,2/5] dt-bindings: net: phy: Document LED inactive high impedance mode https://git.kernel.org/netdev/net-next/c/355c6dc37efa - [net-next,v10,3/5] net: phy: add support for PHY LEDs polarity modes https://git.kernel.org/netdev/net-next/c/7ae215ee7bb8 - [net-next,v10,4/5] dt-bindings: net: Document QCA808x PHYs https://git.kernel.org/netdev/net-next/c/91e893b43d1c - [net-next,v10,5/5] net: phy: at803x: add LED support for qca808x https://git.kernel.org/netdev/net-next/c/7196062b64ee You are awesome, thank you!
> 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
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...