| Message ID | 473d62f268f2a317fd81d0f38f15d2f2f98e2451.1728056697.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next] net: phy: always set polarity_modes if op is supported | expand |
On Fri, Oct 04, 2024 at 04:46:33PM +0100, Daniel Golle wrote: > Some PHYs drive LEDs active-low by default and polarity needs to be > configured in case the 'active-low' property is NOT set. > One way to achieve this without introducing an additional 'active-high' > property would be to always call the led_polarity_set operation if it > is supported by the phy driver. It is a good idea to state why it is RFC. What comments do you want? So this potentially causes regressions/change in behaviour. Strapping or the bootloader could set the polarity, and Linux leaves it alone up until this patch. The device tree binding says: active-low: type: boolean description: Makes LED active low. To turn the LED ON, line needs to be set to low voltage instead of high. There is no mention that the absence of this property means it is active-high. In effect, i think we have a tristate, active-low, active-high, don't touch. I think adding an active-high property is probably the safest bet, even if it is more work. Andrew
On Fri, Oct 04, 2024 at 10:59:45PM +0200, Andrew Lunn wrote: > On Fri, Oct 04, 2024 at 04:46:33PM +0100, Daniel Golle wrote: > > Some PHYs drive LEDs active-low by default and polarity needs to be > > configured in case the 'active-low' property is NOT set. > > One way to achieve this without introducing an additional 'active-high' > > property would be to always call the led_polarity_set operation if it > > is supported by the phy driver. > > It is a good idea to state why it is RFC. What comments do you want? > > [...] > I think adding an active-high property is probably the safest bet, > even if it is more work. Thank you. Exactly that was the clarification I was looking for: If absence of "active-low" would mean "active-high" or should be interpreted as "don't touch". I'll add "active-high" as an additional property then, as I found out that both, Aquantia and Intel/MaxLinear are technically speaking active-low by default (ie. after reset) and what we need to set is a property setting the LED to be driven active-high (ie. driving VDD rather than GND) instead. I hope it's not too late to make this change also for the Aquantia driver.
> I'll add "active-high" as an additional property then, as I found out > that both, Aquantia and Intel/MaxLinear are technically speaking > active-low by default (ie. after reset) and what we need to set is a > property setting the LED to be driven active-high (ie. driving VDD > rather than GND) instead. I hope it's not too late to make this change > also for the Aquantia driver. Adding a new property should not affect backwards compatibility, so it should be safe to merge at any time. Andrew
On Sat, Oct 05, 2024 at 04:17:56PM +0200, Andrew Lunn wrote: > > I'll add "active-high" as an additional property then, as I found out > > that both, Aquantia and Intel/MaxLinear are technically speaking > > active-low by default (ie. after reset) and what we need to set is a > > property setting the LED to be driven active-high (ie. driving VDD > > rather than GND) instead. I hope it's not too late to make this change > > also for the Aquantia driver. > > Adding a new property should not affect backwards compatibility, so it > should be safe to merge at any time. Ok, I will proceed in that direction then and post a patch shortly. My intial assumption that absence of 'active-low' would always imply the LED being driven active-high was due to the commit description of the introduction of the active-low property: commit c94d1783136eb66f2a464a6891a32eeb55eaeacc Author: Christian Marangi <ansuelsmth@gmail.com> Date: Thu Jan 25 21:36:57 2024 +0100 dt-bindings: net: phy: Make LED active-low property common Move LED active-low property to common.yaml. This property is currently defined multiple times by bcm LEDs. This property will now be supported in a generic way for PHY LEDs with the use of a generic function. With active-low bool property not defined, active-high is always assumed. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Acked-by: Lee Jones <lee@kernel.org> Reviewed-by: Rob Herring <robh@kernel.org> Link: https://lore.kernel.org/r/20240125203702.4552-2-ansuelsmth@gmail.com Signed-off-by: Jakub Kicinski <kuba@kernel.org> However, that's not what the code did in the end, it would be either "set active-low" or "don't touch".
On Sat, Oct 05, 2024 at 04:51:30PM +0100, Daniel Golle wrote: > On Sat, Oct 05, 2024 at 04:17:56PM +0200, Andrew Lunn wrote: > > > I'll add "active-high" as an additional property then, as I found out > > > that both, Aquantia and Intel/MaxLinear are technically speaking > > > active-low by default (ie. after reset) and what we need to set is a > > > property setting the LED to be driven active-high (ie. driving VDD > > > rather than GND) instead. I hope it's not too late to make this change > > > also for the Aquantia driver. > > > > Adding a new property should not affect backwards compatibility, so it > > should be safe to merge at any time. > > Ok, I will proceed in that direction then and post a patch shortly. > My intial assumption that absence of 'active-low' would always imply > the LED being driven active-high was due to the commit description of > the introduction of the active-low property: > > commit c94d1783136eb66f2a464a6891a32eeb55eaeacc > Author: Christian Marangi <ansuelsmth@gmail.com> > Date: Thu Jan 25 21:36:57 2024 +0100 > > dt-bindings: net: phy: Make LED active-low property common > > Move LED active-low property to common.yaml. This property is currently > defined multiple times by bcm LEDs. This property will now be supported > in a generic way for PHY LEDs with the use of a generic function. > > With active-low bool property not defined, active-high is always > assumed. So we have a difference between the commit message and what the binding actually says. I would go by what the binding says. However, what about the actual implementations? Do any do what the commit message says? Andrew
On Sat, Oct 05, 2024 at 06:35:58PM +0200, Andrew Lunn wrote: > On Sat, Oct 05, 2024 at 04:51:30PM +0100, Daniel Golle wrote: > > On Sat, Oct 05, 2024 at 04:17:56PM +0200, Andrew Lunn wrote: > > > > I'll add "active-high" as an additional property then, as I found out > > > > that both, Aquantia and Intel/MaxLinear are technically speaking > > > > active-low by default (ie. after reset) and what we need to set is a > > > > property setting the LED to be driven active-high (ie. driving VDD > > > > rather than GND) instead. I hope it's not too late to make this change > > > > also for the Aquantia driver. > > > > > > Adding a new property should not affect backwards compatibility, so it > > > should be safe to merge at any time. > > > > Ok, I will proceed in that direction then and post a patch shortly. > > My intial assumption that absence of 'active-low' would always imply > > the LED being driven active-high was due to the commit description of > > the introduction of the active-low property: > > > > commit c94d1783136eb66f2a464a6891a32eeb55eaeacc > > Author: Christian Marangi <ansuelsmth@gmail.com> > > Date: Thu Jan 25 21:36:57 2024 +0100 > > > > dt-bindings: net: phy: Make LED active-low property common > > > > Move LED active-low property to common.yaml. This property is currently > > defined multiple times by bcm LEDs. This property will now be supported > > in a generic way for PHY LEDs with the use of a generic function. > > > > With active-low bool property not defined, active-high is always > > assumed. > > So we have a difference between the commit message and what the > binding actually says. I would go by what the binding says. +1 > > However, what about the actual implementations? Do any do what the > commit message says? The current implementation for PHY LEDs: - 'active-low' property is present: Change LED polarity (in many cases wrongly from initially being active-low to active-high). - 'active-low' property is not set: Don't touch polarity settings. See drivers/net/phy/phy_device.c, from line 3360: 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; } led_polarity_set() is not called if neither 'active-low' nor 'inactive-high-impedance' are set.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 560e338b307a..d25c61223e83 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -3362,11 +3362,11 @@ static int of_phy_led(struct phy_device *phydev, 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; + /* Return error if asked to set polarity modes but not supported */ + if (modes && !phydev->drv->led_polarity_set) + return -EINVAL; + if (phydev->drv->led_polarity_set) { err = phydev->drv->led_polarity_set(phydev, index, modes); if (err) return err;
Some PHYs drive LEDs active-low by default and polarity needs to be configured in case the 'active-low' property is NOT set. One way to achieve this without introducing an additional 'active-high' property would be to always call the led_polarity_set operation if it is supported by the phy driver. Signed-off-by: Daniel Golle <daniel@makrotopia.org> --- drivers/net/phy/phy_device.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)