This is a bit embarassing and totally my fault so sorry for that!
While reworking the patch to phy_modify API, it was done a logic
error and made the brightness_set function broken. It wasn't
notice in last revisions test as the testing method was to verify
if hw control was correctly working.
Noticing this problem also made me notice an additional problem
with the polarity.
The introduced patch made the polarity configurable but I forgot
to add the required code to enable Active High by default.
(the PHY sets active low by default)
This wasn't notice with hw control testing as the LED blink on
traffic and polarity problem are not notice.
It might be worth discussing if needed a change in implementation
where the polarity function is always called but I think it's
better this way where specific PHY apply fixup with the help
of priv struct and on the config_init phase.
Christian Marangi (2):
net: phy: qcom: qca808x: fix logic error in LED brightness set
net: phy: qcom: qca808x: default to LED active High if not set
drivers/net/phy/qcom/qca808x.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
--
2.43.0
In switching to using phy_modify_mmd and a more short version of the
LED ON/OFF condition in later revision, it was made a logic error where
value ? QCA808X_LED_FORCE_ON : QCA808X_LED_FORCE_OFF is always true as
value is always OR with QCA808X_LED_FORCE_EN due to missing ()
resulting in the testing condition being QCA808X_LED_FORCE_EN | value.
Add the () to apply the correct condition and restore correct
functionality of the brightness ON/OFF.
Fixes: 7196062b64ee ("net: phy: at803x: add LED support for qca808x")
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/qcom/qca808x.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
index 0f549027d899..00db4b274264 100644
--- a/drivers/net/phy/qcom/qca808x.c
+++ b/drivers/net/phy/qcom/qca808x.c
@@ -820,8 +820,8 @@ static int qca808x_led_brightness_set(struct phy_device *phydev,
return phy_modify_mmd(phydev, MDIO_MMD_AN, reg,
QCA808X_LED_FORCE_EN | QCA808X_LED_FORCE_MODE_MASK,
- QCA808X_LED_FORCE_EN | value ? QCA808X_LED_FORCE_ON :
- QCA808X_LED_FORCE_OFF);
+ QCA808X_LED_FORCE_EN | (value ? QCA808X_LED_FORCE_ON :
+ QCA808X_LED_FORCE_OFF));
}
static int qca808x_led_blink_set(struct phy_device *phydev, u8 index,
--
2.43.0
qca808x PHY provide support for the led_polarity_set OP to configure
and apply the active-low property but on PHY reset, the Active High bit
is not set resulting in the LED driven as active-low.
To fix this, check if active-low is not set in DT and enable Active High
polarity by default to restore correct funcionality of the LED.
Fixes: 7196062b64ee ("net: phy: at803x: add LED support for qca808x")
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/net/phy/qcom/qca808x.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/phy/qcom/qca808x.c b/drivers/net/phy/qcom/qca808x.c
index 00db4b274264..d88e04278fc4 100644
--- a/drivers/net/phy/qcom/qca808x.c
+++ b/drivers/net/phy/qcom/qca808x.c
@@ -290,8 +290,18 @@ static int qca808x_probe(struct phy_device *phydev)
static int qca808x_config_init(struct phy_device *phydev)
{
+ struct qca808x_priv *priv = phydev->priv;
int ret;
+ /* Default to LED Active High if active-low not in DT */
+ if (priv->led_polarity_mode == -1) {
+ ret = phy_set_bits_mmd(phydev, MDIO_MMD_AN,
+ QCA808X_MMD7_LED_POLARITY_CTRL,
+ QCA808X_LED_ACTIVE_HIGH);
+ if (ret)
+ return ret;
+ }
+
/* Active adc&vga on 802.3az for the link 1000M and 100M */
ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_ADDR_CLD_CTRL7,
QCA808X_8023AZ_AFE_CTRL_MASK, QCA808X_8023AZ_AFE_EN);
--
2.43.0
Hello:
This series was applied to netdev/net-next.git (main)
by David S. Miller <[email protected]>:
On Thu, 1 Feb 2024 14:45:59 +0100 you wrote:
> This is a bit embarassing and totally my fault so sorry for that!
>
> While reworking the patch to phy_modify API, it was done a logic
> error and made the brightness_set function broken. It wasn't
> notice in last revisions test as the testing method was to verify
> if hw control was correctly working.
>
> [...]
Here is the summary with links:
- [net-next,1/2] net: phy: qcom: qca808x: fix logic error in LED brightness set
https://git.kernel.org/netdev/net-next/c/f2ec98566775
- [net-next,2/2] net: phy: qcom: qca808x: default to LED active High if not set
https://git.kernel.org/netdev/net-next/c/f203c8c77c76
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html