| Message ID | 20240419-adin-pin-polarity-v1-2-eaae8708db8d@solid-run.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: adin: add support for setting led-, link-status-pin polarity | expand |
On Fri, Apr 19, 2024 at 05:35:18PM +0200, Josua Mayer wrote: > ADIN1300 supports software control over pin polarity for both LED_0 and > LINK_ST pins. > > Configure the polarity during probe based on device-tree properties. > > Led polarity is only set if specified in device-tree, otherwise the phy > can choose either active-low or active-high based on external line > voltage. Link-status polarity is set to active-high as default if not > specified, which is always the reset-default. > > Signed-off-by: Josua Mayer <josua@solid-run.com> Hi Josua Please take a look at: commit 447b80a9330ef2d9a94fc5a9bf35b6eac061f38b Author: Alexander Stein <alexander.stein@ew.tq-group.com> Date: Wed Jan 31 08:50:48 2024 +0100 net: phy: dp83867: Add support for active-low LEDs Add the led_polarity_set callback for setting LED polarity. Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Signed-off-by: David S. Miller <davem@davemloft.net> Andrew --- pw-bot: cr
Am 19.04.24 um 17:47 schrieb Andrew Lunn: > On Fri, Apr 19, 2024 at 05:35:18PM +0200, Josua Mayer wrote: >> ADIN1300 supports software control over pin polarity for both LED_0 and >> LINK_ST pins. >> >> Configure the polarity during probe based on device-tree properties. >> >> Led polarity is only set if specified in device-tree, otherwise the phy >> can choose either active-low or active-high based on external line >> voltage. Link-status polarity is set to active-high as default if not >> specified, which is always the reset-default. >> >> Signed-off-by: Josua Mayer <josua@solid-run.com> > Hi Josua > > Please take a look at: > > commit 447b80a9330ef2d9a94fc5a9bf35b6eac061f38b > Author: Alexander Stein <alexander.stein@ew.tq-group.com> > Date: Wed Jan 31 08:50:48 2024 +0100 > > net: phy: dp83867: Add support for active-low LEDs > > Add the led_polarity_set callback for setting LED polarity. > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: David S. Miller <davem@davemloft.net> > > > Andrew > > --- Hi Andrew, That looks very much related! I was already planning to investigate adding led support ... . 1. for the LINK_ST pin I believe we still need a non-led-framework device property for setting polarity, as it is a fixed function signal that we can't even turn on or off from software. 2. LED_0 control not currently supported by adin driver. The phy supports what data-sheet calls extended configuration (disabled by default) for controlling led state (on, off, patterns). Since it is not default, I see the polarity setting separate from leds. However I do believe the led_polarity_set callback is an acceptable solution. I might prepare a reduced v2 for only the fixed-function link-status pin. sincerely Josua Mayer
> Hi Andrew, > > That looks very much related! > > I was already planning to investigate adding led support ... . > > 1. for the LINK_ST pin I believe we still need a non-led-framework > device property for setting polarity, as it is a fixed function signal > that we can't even turn on or off from software. We should separate the device tree binding from the implementation of the binding. The binding describes the hardware. The hardware is active low. And the binding can describe that. So i don't see a need for anything vendor specific. I think the real question is, can the current generic LED code be made to handle this LED, or should you have code in the PHY driver to handle this binding in a specific way for this LED? Given the restrictions on this LED, i don't think it makes sense to expose it in /sys/class/leds. But is it possible to leverage the framework to parse the binding and call the polarity function? > 2. LED_0 control not currently supported by adin driver. > The phy supports what data-sheet calls extended configuration > (disabled by default) for controlling led state (on, off, patterns). > > Since it is not default, I see the polarity setting separate from leds. > However I do believe the led_polarity_set callback is an acceptable > solution. Again, you should use the LED binding, even if you don't use the LED framework. I just wounder how much code you will duplicate if you do decide to implement the binding in the driver. And when you fully implement the control of the LED using the framework, are you going to throw the code away again? Andrew
diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c index 2e1a46e121d9..53159dea6381 100644 --- a/drivers/net/phy/adin.c +++ b/drivers/net/phy/adin.c @@ -114,6 +114,9 @@ #define ADIN1300_CDIAG_FLT_DIST(x) (0xba21 + (x)) +#define ADIN1300_LED_A_INV_EN_REG 0xbc01 +#define ADIN1300_LED_A_INV_EN BIT(0) + #define ADIN1300_GE_SOFT_RESET_REG 0xff0c #define ADIN1300_GE_SOFT_RESET BIT(0) @@ -158,6 +161,9 @@ #define ADIN1300_RMII_20_BITS 0x0004 #define ADIN1300_RMII_24_BITS 0x0005 +#define ADIN1300_GE_LNK_STAT_INV_EN_REG 0xff3c +#define ADIN1300_GE_LNK_STAT_INV_EN BIT(0) + /** * struct adin_cfg_reg_map - map a config value to aregister value * @cfg: value in device configuration @@ -522,6 +528,49 @@ static int adin_config_clk_out(struct phy_device *phydev) ADIN1300_GE_CLK_CFG_MASK, sel); } +static int adin_config_pin_polarity(struct phy_device *phydev) +{ + struct device *dev = &phydev->mdio.dev; + int ret; + u32 val; + + /* set led polarity, if property present */ + if (device_property_present(dev, "adi,led-polarity")) { + ret = device_property_read_u32(dev, "adi,led-polarity", &val); + if (ret) { + return ret; + } else if (val > 1) { + phydev_err(phydev, "invalid adi,led-polarity\n"); + return -EINVAL; + } + + ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, + ADIN1300_LED_A_INV_EN_REG, + ADIN1300_LED_A_INV_EN, val); + if (ret) + return ret; + } + + /* set link-status polarity, default to active-high (0) */ + if (device_property_present(dev, "adi,link-st-polarity")) { + ret = device_property_read_u32(dev, "adi,link-st-polarity", &val); + if (ret) { + return ret; + } else if (val > 1) { + phydev_err(phydev, "invalid adi,link-st-polarity\n"); + return -EINVAL; + } + } else { + val = 0; + } + + ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, + ADIN1300_GE_LNK_STAT_INV_EN_REG, + ADIN1300_GE_LNK_STAT_INV_EN, val); + + return ret; +} + static int adin_config_init(struct phy_device *phydev) { int rc; @@ -548,6 +597,10 @@ static int adin_config_init(struct phy_device *phydev) if (rc < 0) return rc; + rc = adin_config_pin_polarity(phydev); + if (rc < 0) + return rc; + phydev_dbg(phydev, "PHY is using mode '%s'\n", phy_modes(phydev->interface));
ADIN1300 supports software control over pin polarity for both LED_0 and LINK_ST pins. Configure the polarity during probe based on device-tree properties. Led polarity is only set if specified in device-tree, otherwise the phy can choose either active-low or active-high based on external line voltage. Link-status polarity is set to active-high as default if not specified, which is always the reset-default. Signed-off-by: Josua Mayer <josua@solid-run.com> --- drivers/net/phy/adin.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)