| Message ID | 20241217-dp83822-leds-v1-1-800b24461013@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: dp83822: Add support for PHY LEDs on DP83822 | expand |
On Tue, Dec 17, 2024 at 10:16:03AM +0100, Dimitri Fedrau wrote: > The DP83822 supports up to three configurable Light Emitting Diode (LED) > pins: LED_0, LED_1 (GPIO1), COL (GPIO2) and RX_D3 (GPIO3). Several > functions can be multiplexed onto the LEDs for different modes of > operation. LED_0 and COL (GPIO2) use the MLED function. MLED can be routed > to only one of these two pins at a time. Add minimal LED controller driver > supporting the most common uses with the 'netdev' trigger. > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > --- > drivers/net/phy/dp83822.c | 271 +++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 269 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c ... > +static int dp83822_led_hw_control_set(struct phy_device *phydev, u8 index, > + unsigned long rules) > +{ > + int mode; > + > + mode = dp83822_led_mode(index, rules); > + if (mode < 0) > + return mode; > + > + if (index == DP83822_LED_INDEX_LED_0 || index == DP83822_LED_INDEX_COL_GPIO2) > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, > + MII_DP83822_MLEDCR, DP83822_MLEDCR_CFG, > + FIELD_PREP(DP83822_MLEDCR_CFG, mode)); ... > +} > + > +static int dp83822_led_hw_control_get(struct phy_device *phydev, u8 index, > + unsigned long *rules) > +{ > + int val; > + > + if (index == DP83822_LED_INDEX_LED_0 || DP83822_LED_INDEX_COL_GPIO2) { Hi Dimitri, As per the condition near the top of dp83822_led_hw_control_set(), should this be: if (index == DP83822_LED_INDEX_LED_0 || index == DP83822_LED_INDEX_COL_GPIO2) { Flagged by W=1 + -Wno-error build with clang-19. drivers/net/phy/dp83822.c:1029:39: note: use '|' for a bitwise operation 1029 | if (index == DP83822_LED_INDEX_LED_0 || DP83822_LED_INDEX_COL_GPIO2) { | ^~ | ...
> +static int dp83822_led_hw_control_set(struct phy_device *phydev, u8 index, > + unsigned long rules) > +{ > + int mode; > + > + mode = dp83822_led_mode(index, rules); > + if (mode < 0) > + return mode; > + > + if (index == DP83822_LED_INDEX_LED_0 || index == DP83822_LED_INDEX_COL_GPIO2) > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, > + MII_DP83822_MLEDCR, DP83822_MLEDCR_CFG, > + FIELD_PREP(DP83822_MLEDCR_CFG, mode)); > + else if (index == DP83822_LED_INDEX_LED_1_GPIO1) > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, > + MII_DP83822_LEDCFG1, > + DP83822_LEDCFG1_LED1_CTRL, > + FIELD_PREP(DP83822_LEDCFG1_LED1_CTRL, > + mode)); > + else > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, > + MII_DP83822_LEDCFG1, > + DP83822_LEDCFG1_LED3_CTRL, > + FIELD_PREP(DP83822_LEDCFG1_LED3_CTRL, > + mode)); index is taken direct from DT. Somebody might have: leds { #address-cells = <1>; #size-cells = <0>; led@42 { reg = <42>; color = <LED_COLOR_ID_WHITE>; function = LED_FUNCTION_LAN; default-state = "keep"; }; }; so you should not assume if it is not 0, 1 or 2, then it must be 3. Please always validate index. Andrew --- pw-bot: cr
Am Tue, Dec 17, 2024 at 04:32:08PM +0000 schrieb Simon Horman: > On Tue, Dec 17, 2024 at 10:16:03AM +0100, Dimitri Fedrau wrote: > > The DP83822 supports up to three configurable Light Emitting Diode (LED) > > pins: LED_0, LED_1 (GPIO1), COL (GPIO2) and RX_D3 (GPIO3). Several > > functions can be multiplexed onto the LEDs for different modes of > > operation. LED_0 and COL (GPIO2) use the MLED function. MLED can be routed > > to only one of these two pins at a time. Add minimal LED controller driver > > supporting the most common uses with the 'netdev' trigger. > > > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > --- > > drivers/net/phy/dp83822.c | 271 +++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 269 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c > > ... > > > +static int dp83822_led_hw_control_set(struct phy_device *phydev, u8 index, > > + unsigned long rules) > > +{ > > + int mode; > > + > > + mode = dp83822_led_mode(index, rules); > > + if (mode < 0) > > + return mode; > > + > > + if (index == DP83822_LED_INDEX_LED_0 || index == DP83822_LED_INDEX_COL_GPIO2) > > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, > > + MII_DP83822_MLEDCR, DP83822_MLEDCR_CFG, > > + FIELD_PREP(DP83822_MLEDCR_CFG, mode)); > > ... > > > +} > > + > > +static int dp83822_led_hw_control_get(struct phy_device *phydev, u8 index, > > + unsigned long *rules) > > +{ > > + int val; > > + > > + if (index == DP83822_LED_INDEX_LED_0 || DP83822_LED_INDEX_COL_GPIO2) { > > Hi Dimitri, > > As per the condition near the top of dp83822_led_hw_control_set(), should > this be: > > if (index == DP83822_LED_INDEX_LED_0 || > index == DP83822_LED_INDEX_COL_GPIO2) { > > Flagged by W=1 + -Wno-error build with clang-19. > > drivers/net/phy/dp83822.c:1029:39: note: use '|' for a bitwise operation > 1029 | if (index == DP83822_LED_INDEX_LED_0 || DP83822_LED_INDEX_COL_GPIO2) { > | ^~ > | > > ... Thanks, will fix it. Best regards, Dimitri
Am Tue, Dec 17, 2024 at 06:13:25PM +0100 schrieb Andrew Lunn: > > +static int dp83822_led_hw_control_set(struct phy_device *phydev, u8 index, > > + unsigned long rules) > > +{ > > + int mode; > > + > > + mode = dp83822_led_mode(index, rules); > > + if (mode < 0) > > + return mode; > > + > > + if (index == DP83822_LED_INDEX_LED_0 || index == DP83822_LED_INDEX_COL_GPIO2) > > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, > > + MII_DP83822_MLEDCR, DP83822_MLEDCR_CFG, > > + FIELD_PREP(DP83822_MLEDCR_CFG, mode)); > > + else if (index == DP83822_LED_INDEX_LED_1_GPIO1) > > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, > > + MII_DP83822_LEDCFG1, > > + DP83822_LEDCFG1_LED1_CTRL, > > + FIELD_PREP(DP83822_LEDCFG1_LED1_CTRL, > > + mode)); > > + else > > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, > > + MII_DP83822_LEDCFG1, > > + DP83822_LEDCFG1_LED3_CTRL, > > + FIELD_PREP(DP83822_LEDCFG1_LED3_CTRL, > > + mode)); > > index is taken direct from DT. Somebody might have: > > leds { > #address-cells = <1>; > #size-cells = <0>; > > led@42 { > reg = <42>; > color = <LED_COLOR_ID_WHITE>; > function = LED_FUNCTION_LAN; > default-state = "keep"; > }; > }; > > so you should not assume if it is not 0, 1 or 2, then it must be > 3. Please always validate index. > dp83822_of_init_leds does a check that index is 0, 1, 2 or 3. Is this sufficient ? Otherwise I would validate the index. Best regards, Dimitri
Hi Dimitri,
kernel test robot noticed the following build warnings:
[auto build test WARNING on a14a429069bb1a18eb9fe63d68fcaa77dffe0e23]
url: https://github.com/intel-lab-lkp/linux/commits/Dimitri-Fedrau/net-phy-dp83822-Add-support-for-PHY-LEDs-on-DP83822/20241217-172305
base: a14a429069bb1a18eb9fe63d68fcaa77dffe0e23
patch link: https://lore.kernel.org/r/20241217-dp83822-leds-v1-1-800b24461013%40gmail.com
patch subject: [PATCH net-next] net: phy: dp83822: Add support for PHY LEDs on DP83822
config: x86_64-buildonly-randconfig-002-20241218 (https://download.01.org/0day-ci/archive/20241218/202412181638.7XsHSwtp-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241218/202412181638.7XsHSwtp-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412181638.7XsHSwtp-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/net/phy/dp83822.c:7:
In file included from include/linux/ethtool.h:18:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:8:
In file included from include/linux/cacheflush.h:5:
In file included from arch/x86/include/asm/cacheflush.h:5:
In file included from include/linux/mm.h:2223:
include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
504 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
505 | item];
| ~~~~
include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
511 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
512 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
524 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
525 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/phy/dp83822.c:1029:39: warning: use of logical '||' with constant operand [-Wconstant-logical-operand]
1029 | if (index == DP83822_LED_INDEX_LED_0 || DP83822_LED_INDEX_COL_GPIO2) {
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/phy/dp83822.c:1029:39: note: use '|' for a bitwise operation
1029 | if (index == DP83822_LED_INDEX_LED_0 || DP83822_LED_INDEX_COL_GPIO2) {
| ^~
| |
5 warnings generated.
vim +1029 drivers/net/phy/dp83822.c
1023
1024 static int dp83822_led_hw_control_get(struct phy_device *phydev, u8 index,
1025 unsigned long *rules)
1026 {
1027 int val;
1028
> 1029 if (index == DP83822_LED_INDEX_LED_0 || DP83822_LED_INDEX_COL_GPIO2) {
1030 val = phy_read_mmd(phydev, MDIO_MMD_VEND2, MII_DP83822_MLEDCR);
1031 if (val < 0)
1032 return val;
1033
1034 val = FIELD_GET(DP83822_MLEDCR_CFG, val);
1035 } else {
1036 val = phy_read_mmd(phydev, MDIO_MMD_VEND2, MII_DP83822_LEDCFG1);
1037 if (val < 0)
1038 return val;
1039
1040 if (index == DP83822_LED_INDEX_LED_1_GPIO1)
1041 val = FIELD_GET(DP83822_LEDCFG1_LED1_CTRL, val);
1042 else
1043 val = FIELD_GET(DP83822_LEDCFG1_LED3_CTRL, val);
1044 }
1045
1046 switch (val) {
1047 case DP83822_LED_FN_LINK:
1048 *rules = BIT(TRIGGER_NETDEV_LINK);
1049 break;
1050 case DP83822_LED_FN_LINK_10_BT:
1051 *rules = BIT(TRIGGER_NETDEV_LINK_10);
1052 break;
1053 case DP83822_LED_FN_LINK_100_BTX:
1054 *rules = BIT(TRIGGER_NETDEV_LINK_100);
1055 break;
1056 case DP83822_LED_FN_FULL_DUPLEX:
1057 *rules = BIT(TRIGGER_NETDEV_FULL_DUPLEX);
1058 break;
1059 case DP83822_LED_FN_TX:
1060 *rules = BIT(TRIGGER_NETDEV_TX);
1061 break;
1062 case DP83822_LED_FN_RX:
1063 *rules = BIT(TRIGGER_NETDEV_RX);
1064 break;
1065 case DP83822_LED_FN_RX_TX:
1066 *rules = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX);
1067 break;
1068 case DP83822_LED_FN_RX_TX_ERR:
1069 *rules = BIT(TRIGGER_NETDEV_TX_ERR) | BIT(TRIGGER_NETDEV_RX_ERR);
1070 break;
1071 case DP83822_LED_FN_LINK_RX_TX:
1072 *rules = BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) |
1073 BIT(TRIGGER_NETDEV_RX);
1074 break;
1075 default:
1076 *rules = 0;
1077 break;
1078 }
1079
1080 return 0;
1081 }
1082
Am Tue, Dec 17, 2024 at 06:13:25PM +0100 schrieb Andrew Lunn: > > +static int dp83822_led_hw_control_set(struct phy_device *phydev, u8 index, > > + unsigned long rules) > > +{ > > + int mode; > > + > > + mode = dp83822_led_mode(index, rules); > > + if (mode < 0) > > + return mode; > > + > > + if (index == DP83822_LED_INDEX_LED_0 || index == DP83822_LED_INDEX_COL_GPIO2) > > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, > > + MII_DP83822_MLEDCR, DP83822_MLEDCR_CFG, > > + FIELD_PREP(DP83822_MLEDCR_CFG, mode)); > > + else if (index == DP83822_LED_INDEX_LED_1_GPIO1) > > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, > > + MII_DP83822_LEDCFG1, > > + DP83822_LEDCFG1_LED1_CTRL, > > + FIELD_PREP(DP83822_LEDCFG1_LED1_CTRL, > > + mode)); > > + else > > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, > > + MII_DP83822_LEDCFG1, > > + DP83822_LEDCFG1_LED3_CTRL, > > + FIELD_PREP(DP83822_LEDCFG1_LED3_CTRL, > > + mode)); > > index is taken direct from DT. Somebody might have: > > leds { > #address-cells = <1>; > #size-cells = <0>; > > led@42 { > reg = <42>; > color = <LED_COLOR_ID_WHITE>; > function = LED_FUNCTION_LAN; > default-state = "keep"; > }; > }; > > so you should not assume if it is not 0, 1 or 2, then it must be > 3. Please always validate index. > By the way. Wouldn't it be helpful adding a u32 max_leds to struct phy_driver ? Every driver supporting PHY LEDs validates index at the moment. With max_leds it should be easy to check it in of_phy_leds and return with an error if index is not valid. Best regards, Dimitri
> > index is taken direct from DT. Somebody might have: > > > > leds { > > #address-cells = <1>; > > #size-cells = <0>; > > > > led@42 { > > reg = <42>; > > color = <LED_COLOR_ID_WHITE>; > > function = LED_FUNCTION_LAN; > > default-state = "keep"; > > }; > > }; > > > > so you should not assume if it is not 0, 1 or 2, then it must be > > 3. Please always validate index. > > > dp83822_of_init_leds does a check that index is 0, 1, 2 or 3. Is this > sufficient ? Otherwise I would validate the index. Ah, i missed that. I'm just used to the usual pattern that every PHY driver has a check for the MAX LEDs in their callback. Andrew
> By the way. Wouldn't it be helpful adding a u32 max_leds to > struct phy_driver ? Every driver supporting PHY LEDs validates index at the > moment. With max_leds it should be easy to check it in of_phy_leds and > return with an error if index is not valid. I have been considering it. However, so far developers have been good at adding the checks, because the first driver had the checks, cargo cult at its best. If we are going to add it, we should do it early, before there are too many PHY drivers which need updating. Andrew
Am Wed, Dec 18, 2024 at 06:16:20PM +0100 schrieb Andrew Lunn: > > By the way. Wouldn't it be helpful adding a u32 max_leds to > > struct phy_driver ? Every driver supporting PHY LEDs validates index at the > > moment. With max_leds it should be easy to check it in of_phy_leds and > > return with an error if index is not valid. > > I have been considering it. However, so far developers have been good > at adding the checks, because the first driver had the checks, cargo > cult at its best. > > If we are going to add it, we should do it early, before there are too > many PHY drivers which need updating. > Another solution without breaking others driver would be to add a callback in struct phy_driver: int (*led_validate_index)(struct phy_device *dev, int index) It should be called in of_phy_led right after reading in reg property: if (phydev->drv->led_validate_index) ret = phydev->drv->led_validate_index(phydev, index); This would solve another isssue I have. The LED pins of the DP83822 can be multiplexed. Not all of them have per default a LED function. So I need to set them up. In dp83822_of_init_leds I iterate over all DT nodes in leds to get the information which of the pins should output LED function. Using the callback would eleminate the need for copying code of functions of_phy_leds and of_phy_led. I can come up with a patch if you agree. I would add it to the patch series and then we would also have a example with the DP83822. Best regards, Dimitri
On Wed, Dec 18, 2024 at 07:17:52PM +0100, Dimitri Fedrau wrote: > Am Wed, Dec 18, 2024 at 06:16:20PM +0100 schrieb Andrew Lunn: > > > By the way. Wouldn't it be helpful adding a u32 max_leds to > > > struct phy_driver ? Every driver supporting PHY LEDs validates index at the > > > moment. With max_leds it should be easy to check it in of_phy_leds and > > > return with an error if index is not valid. > > > > I have been considering it. However, so far developers have been good > > at adding the checks, because the first driver had the checks, cargo > > cult at its best. > > > > If we are going to add it, we should do it early, before there are too > > many PHY drivers which need updating. > > > Another solution without breaking others driver would be to add a > callback in struct phy_driver: Adding the maximum number of LEDs to struct phy_driver will not break anything. But we would want to remove all the tests for the index value from the drivers, since they become pointless. That will be easier to do when there are less drivers which need editing. > int (*led_validate_index)(struct phy_device *dev, int index) > It should be called in of_phy_led right after reading in reg property: > if (phydev->drv->led_validate_index) > ret = phydev->drv->led_validate_index(phydev, index); > > This would solve another isssue I have. The LED pins of the DP83822 can > be multiplexed. Not all of them have per default a LED function. So I > need to set them up. In dp83822_of_init_leds I iterate over all DT nodes > in leds to get the information which of the pins should output LED > function. Using the callback would eleminate the need for copying code of > functions of_phy_leds and of_phy_led. Your hardware is pretty unique. It might be best to keep it in the driver, until there is a second driver which needs the same. I also think you need the complete configuration in order to validate it, not each LED one by one, which your led_validate_index() would provide. Andrew
Am Wed, Dec 18, 2024 at 09:19:47PM +0100 schrieb Andrew Lunn: > On Wed, Dec 18, 2024 at 07:17:52PM +0100, Dimitri Fedrau wrote: > > Am Wed, Dec 18, 2024 at 06:16:20PM +0100 schrieb Andrew Lunn: > > > > By the way. Wouldn't it be helpful adding a u32 max_leds to > > > > struct phy_driver ? Every driver supporting PHY LEDs validates index at the > > > > moment. With max_leds it should be easy to check it in of_phy_leds and > > > > return with an error if index is not valid. > > > > > > I have been considering it. However, so far developers have been good > > > at adding the checks, because the first driver had the checks, cargo > > > cult at its best. > > > > > > If we are going to add it, we should do it early, before there are too > > > many PHY drivers which need updating. > > > > > Another solution without breaking others driver would be to add a > > callback in struct phy_driver: > > Adding the maximum number of LEDs to struct phy_driver will not break > anything. But we would want to remove all the tests for the index > value from the drivers, since they become pointless. That will be > easier to do when there are less drivers which need editing. > Just adding the number will not break anything, but probably the test that should be implemented in of_phy_led. Except the test gets skipped if maximum number of LEDs is not set(zero). Otherwise we would have to change all existing drivers to set the maximum numbers of LEDs. > > int (*led_validate_index)(struct phy_device *dev, int index) > > It should be called in of_phy_led right after reading in reg property: > > if (phydev->drv->led_validate_index) > > ret = phydev->drv->led_validate_index(phydev, index); > > > > This would solve another isssue I have. The LED pins of the DP83822 can > > be multiplexed. Not all of them have per default a LED function. So I > > need to set them up. In dp83822_of_init_leds I iterate over all DT nodes > > in leds to get the information which of the pins should output LED > > function. Using the callback would eleminate the need for copying code of > > functions of_phy_leds and of_phy_led. > > Your hardware is pretty unique. It might be best to keep it in the > driver, until there is a second driver which needs the same. I also > think you need the complete configuration in order to validate it, not > each LED one by one, which your led_validate_index() would provide. > I have implemented LED support for marvell-88q2xxx.c which I wanted to upstream after LED support for DP83822 is done. There I have the same issue. You are right, I need the whole configuration. But at the moment I read it in the same way as it is done in of_phy_led and save indices to bool led_pin_enable[DP83822_MAX_LED_PINS] and set them up later in dp83822_config_init. How do we proceed ? Implement maximum numbers of LEDs ? Skip the validation index callback when upstreaming LED support for marvell-88q2xxx.c ? Best regards, Dimitri
diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c index 334c17a68edd7c2a0f707b3ccafaa7a870818fbe..66651dbbd944fe1469057e9ef2f2057aedfb9e29 100644 --- a/drivers/net/phy/dp83822.c +++ b/drivers/net/phy/dp83822.c @@ -30,6 +30,9 @@ #define MII_DP83822_FCSCR 0x14 #define MII_DP83822_RCSR 0x17 #define MII_DP83822_RESET_CTRL 0x1f +#define MII_DP83822_MLEDCR 0x25 +#define MII_DP83822_LEDCFG1 0x460 +#define MII_DP83822_IOCTRL1 0x462 #define MII_DP83822_IOCTRL2 0x463 #define MII_DP83822_GENCFG 0x465 #define MII_DP83822_SOR1 0x467 @@ -105,10 +108,26 @@ #define DP83822_RX_CLK_SHIFT BIT(12) #define DP83822_TX_CLK_SHIFT BIT(11) +/* MLEDCR bits */ +#define DP83822_MLEDCR_CFG GENMASK(6, 3) +#define DP83822_MLEDCR_ROUTE GENMASK(1, 0) +#define DP83822_MLEDCR_ROUTE_LED_0 DP83822_MLEDCR_ROUTE + +/* LEDCFG1 bits */ +#define DP83822_LEDCFG1_LED1_CTRL GENMASK(11, 8) +#define DP83822_LEDCFG1_LED3_CTRL GENMASK(7, 4) + +/* IOCTRL1 bits */ +#define DP83822_IOCTRL1_GPIO3_CTRL GENMASK(10, 8) +#define DP83822_IOCTRL1_GPIO3_CTRL_LED3 BIT(0) +#define DP83822_IOCTRL1_GPIO1_CTRL GENMASK(2, 0) +#define DP83822_IOCTRL1_GPIO1_CTRL_LED_1 BIT(0) + /* IOCTRL2 bits */ #define DP83822_IOCTRL2_GPIO2_CLK_SRC GENMASK(6, 4) #define DP83822_IOCTRL2_GPIO2_CTRL GENMASK(2, 0) #define DP83822_IOCTRL2_GPIO2_CTRL_CLK_REF GENMASK(1, 0) +#define DP83822_IOCTRL2_GPIO2_CTRL_MLED BIT(0) #define DP83822_CLK_SRC_MAC_IF 0x0 #define DP83822_CLK_SRC_XI 0x1 @@ -117,6 +136,22 @@ #define DP83822_CLK_SRC_FREE_RUNNING 0x6 #define DP83822_CLK_SRC_RECOVERED 0x7 +#define DP83822_LED_FN_LINK 0x0 /* Link established */ +#define DP83822_LED_FN_RX_TX 0x1 /* Receive or Transmit activity */ +#define DP83822_LED_FN_TX 0x2 /* Transmit activity */ +#define DP83822_LED_FN_RX 0x3 /* Receive activity */ +#define DP83822_LED_FN_COLLISION 0x4 /* Collision detected */ +#define DP83822_LED_FN_LINK_100_BTX 0x5 /* 100 BTX link established */ +#define DP83822_LED_FN_LINK_10_BT 0x6 /* 10BT link established */ +#define DP83822_LED_FN_FULL_DUPLEX 0x7 /* Full duplex */ +#define DP83822_LED_FN_LINK_RX_TX 0x8 /* Link established, blink for rx or tx activity */ +#define DP83822_LED_FN_ACTIVE_STRETCH 0x9 /* Active Stretch Signal */ +#define DP83822_LED_FN_MII_LINK 0xa /* MII LINK (100BT+FD) */ +#define DP83822_LED_FN_LPI_MODE 0xb /* LPI Mode (EEE) */ +#define DP83822_LED_FN_RX_TX_ERR 0xc /* TX/RX MII Error */ +#define DP83822_LED_FN_LINK_LOST 0xd /* Link Lost */ +#define DP83822_LED_FN_PRBS_ERR 0xe /* Blink for PRBS error */ + /* SOR1 mode */ #define DP83822_STRAP_MODE1 0 #define DP83822_STRAP_MODE2 BIT(0) @@ -145,6 +180,12 @@ ADVERTISED_FIBRE | \ ADVERTISED_Pause | ADVERTISED_Asym_Pause) +#define DP83822_MAX_LED_PINS 4 + +#define DP83822_LED_INDEX_LED_0 0 +#define DP83822_LED_INDEX_LED_1_GPIO1 1 +#define DP83822_LED_INDEX_COL_GPIO2 2 +#define DP83822_LED_INDEX_RX_D3_GPIO3 3 struct dp83822_private { bool fx_signal_det_low; int fx_enabled; @@ -154,6 +195,7 @@ struct dp83822_private { struct ethtool_wolinfo wol; bool set_gpio2_clk_out; u32 gpio2_clk_out; + bool led_pin_enable[DP83822_MAX_LED_PINS]; }; static int dp83822_config_wol(struct phy_device *phydev, @@ -418,6 +460,48 @@ static int dp83822_read_status(struct phy_device *phydev) return 0; } +static int dp83822_config_init_leds(struct phy_device *phydev) +{ + struct dp83822_private *dp83822 = phydev->priv; + int ret; + + if (dp83822->led_pin_enable[DP83822_LED_INDEX_LED_0]) { + ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, MII_DP83822_MLEDCR, + DP83822_MLEDCR_ROUTE, + FIELD_PREP(DP83822_MLEDCR_ROUTE, + DP83822_MLEDCR_ROUTE_LED_0)); + if (ret) + return ret; + } else if (dp83822->led_pin_enable[DP83822_LED_INDEX_COL_GPIO2]) { + ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, MII_DP83822_IOCTRL2, + DP83822_IOCTRL2_GPIO2_CTRL, + FIELD_PREP(DP83822_IOCTRL2_GPIO2_CTRL, + DP83822_IOCTRL2_GPIO2_CTRL_MLED)); + if (ret) + return ret; + } + + if (dp83822->led_pin_enable[DP83822_LED_INDEX_LED_1_GPIO1]) { + ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, MII_DP83822_IOCTRL1, + DP83822_IOCTRL1_GPIO1_CTRL, + FIELD_PREP(DP83822_IOCTRL1_GPIO1_CTRL, + DP83822_IOCTRL1_GPIO1_CTRL_LED_1)); + if (ret) + return ret; + } + + if (dp83822->led_pin_enable[DP83822_LED_INDEX_RX_D3_GPIO3]) { + ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, MII_DP83822_IOCTRL1, + DP83822_IOCTRL1_GPIO3_CTRL, + FIELD_PREP(DP83822_IOCTRL1_GPIO3_CTRL, + DP83822_IOCTRL1_GPIO3_CTRL_LED3)); + if (ret) + return ret; + } + + return 0; +} + static int dp83822_config_init(struct phy_device *phydev) { struct dp83822_private *dp83822 = phydev->priv; @@ -437,6 +521,10 @@ static int dp83822_config_init(struct phy_device *phydev) FIELD_PREP(DP83822_IOCTRL2_GPIO2_CLK_SRC, dp83822->gpio2_clk_out)); + err = dp83822_config_init_leds(phydev); + if (err) + return err; + if (phy_interface_is_rgmii(phydev)) { rx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0, true); @@ -631,6 +719,56 @@ static int dp83822_phy_reset(struct phy_device *phydev) } #ifdef CONFIG_OF_MDIO +static int dp83822_of_init_leds(struct phy_device *phydev) +{ + struct device_node *node = phydev->mdio.dev.of_node; + struct dp83822_private *dp83822 = phydev->priv; + struct device_node *leds; + u32 index; + int err; + + if (!node) + return 0; + + leds = of_get_child_by_name(node, "leds"); + if (!leds) + return 0; + + for_each_available_child_of_node_scoped(leds, led) { + err = of_property_read_u32(led, "reg", &index); + if (err) + return err; + + if (index <= DP83822_LED_INDEX_RX_D3_GPIO3) + dp83822->led_pin_enable[index] = true; + else + return -EINVAL; + } + + /* LED_0 and COL(GPIO2) use the MLED function. MLED can be routed to + * only one of these two pins at a time. + */ + if (dp83822->led_pin_enable[DP83822_LED_INDEX_LED_0] && + dp83822->led_pin_enable[DP83822_LED_INDEX_COL_GPIO2]) { + phydev_err(phydev, "LED_0 and COL(GPIO2) cannot be used as LED output at the same time\n"); + return -EINVAL; + } + + if (dp83822->led_pin_enable[DP83822_LED_INDEX_COL_GPIO2] && + dp83822->set_gpio2_clk_out) { + phydev_err(phydev, "COL(GPIO2) cannot be used as LED outout, already used as clock output\n"); + return -EINVAL; + } + + if (dp83822->led_pin_enable[DP83822_LED_INDEX_RX_D3_GPIO3] && + phydev->interface != PHY_INTERFACE_MODE_RMII) { + phydev_err(phydev, "RX_D3 can only be used as LED output when in RMII mode\n"); + return -EINVAL; + } + + return 0; +} + static int dp83822_of_init(struct phy_device *phydev) { struct dp83822_private *dp83822 = phydev->priv; @@ -671,7 +809,7 @@ static int dp83822_of_init(struct phy_device *phydev) dp83822->set_gpio2_clk_out = true; } - return 0; + return dp83822_of_init_leds(phydev); } static int dp83826_to_dac_minus_one_regval(int percent) @@ -769,7 +907,9 @@ static int dp83822_probe(struct phy_device *phydev) if (ret) return ret; - dp83822_of_init(phydev); + ret = dp83822_of_init(phydev); + if (ret) + return ret; if (dp83822->fx_enabled) phydev->port = PORT_FIBRE; @@ -816,6 +956,130 @@ static int dp83822_resume(struct phy_device *phydev) return 0; } +static int dp83822_led_mode(u8 index, unsigned long rules) +{ + switch (rules) { + case BIT(TRIGGER_NETDEV_LINK): + return DP83822_LED_FN_LINK; + case BIT(TRIGGER_NETDEV_LINK_10): + return DP83822_LED_FN_LINK_10_BT; + case BIT(TRIGGER_NETDEV_LINK_100): + return DP83822_LED_FN_LINK_100_BTX; + case BIT(TRIGGER_NETDEV_FULL_DUPLEX): + return DP83822_LED_FN_FULL_DUPLEX; + case BIT(TRIGGER_NETDEV_TX): + return DP83822_LED_FN_TX; + case BIT(TRIGGER_NETDEV_RX): + return DP83822_LED_FN_RX; + case BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX): + return DP83822_LED_FN_RX_TX; + case BIT(TRIGGER_NETDEV_TX_ERR) | BIT(TRIGGER_NETDEV_RX_ERR): + return DP83822_LED_FN_RX_TX_ERR; + case BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX): + return DP83822_LED_FN_LINK_RX_TX; + default: + return -EOPNOTSUPP; + } +} + +static int dp83822_led_hw_is_supported(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + int mode; + + mode = dp83822_led_mode(index, rules); + if (mode < 0) + return mode; + + return 0; +} + +static int dp83822_led_hw_control_set(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + int mode; + + mode = dp83822_led_mode(index, rules); + if (mode < 0) + return mode; + + if (index == DP83822_LED_INDEX_LED_0 || index == DP83822_LED_INDEX_COL_GPIO2) + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, + MII_DP83822_MLEDCR, DP83822_MLEDCR_CFG, + FIELD_PREP(DP83822_MLEDCR_CFG, mode)); + else if (index == DP83822_LED_INDEX_LED_1_GPIO1) + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, + MII_DP83822_LEDCFG1, + DP83822_LEDCFG1_LED1_CTRL, + FIELD_PREP(DP83822_LEDCFG1_LED1_CTRL, + mode)); + else + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, + MII_DP83822_LEDCFG1, + DP83822_LEDCFG1_LED3_CTRL, + FIELD_PREP(DP83822_LEDCFG1_LED3_CTRL, + mode)); +} + +static int dp83822_led_hw_control_get(struct phy_device *phydev, u8 index, + unsigned long *rules) +{ + int val; + + if (index == DP83822_LED_INDEX_LED_0 || DP83822_LED_INDEX_COL_GPIO2) { + val = phy_read_mmd(phydev, MDIO_MMD_VEND2, MII_DP83822_MLEDCR); + if (val < 0) + return val; + + val = FIELD_GET(DP83822_MLEDCR_CFG, val); + } else { + val = phy_read_mmd(phydev, MDIO_MMD_VEND2, MII_DP83822_LEDCFG1); + if (val < 0) + return val; + + if (index == DP83822_LED_INDEX_LED_1_GPIO1) + val = FIELD_GET(DP83822_LEDCFG1_LED1_CTRL, val); + else + val = FIELD_GET(DP83822_LEDCFG1_LED3_CTRL, val); + } + + switch (val) { + case DP83822_LED_FN_LINK: + *rules = BIT(TRIGGER_NETDEV_LINK); + break; + case DP83822_LED_FN_LINK_10_BT: + *rules = BIT(TRIGGER_NETDEV_LINK_10); + break; + case DP83822_LED_FN_LINK_100_BTX: + *rules = BIT(TRIGGER_NETDEV_LINK_100); + break; + case DP83822_LED_FN_FULL_DUPLEX: + *rules = BIT(TRIGGER_NETDEV_FULL_DUPLEX); + break; + case DP83822_LED_FN_TX: + *rules = BIT(TRIGGER_NETDEV_TX); + break; + case DP83822_LED_FN_RX: + *rules = BIT(TRIGGER_NETDEV_RX); + break; + case DP83822_LED_FN_RX_TX: + *rules = BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX); + break; + case DP83822_LED_FN_RX_TX_ERR: + *rules = BIT(TRIGGER_NETDEV_TX_ERR) | BIT(TRIGGER_NETDEV_RX_ERR); + break; + case DP83822_LED_FN_LINK_RX_TX: + *rules = BIT(TRIGGER_NETDEV_LINK) | BIT(TRIGGER_NETDEV_TX) | + BIT(TRIGGER_NETDEV_RX); + break; + default: + *rules = 0; + break; + } + + return 0; +} + #define DP83822_PHY_DRIVER(_id, _name) \ { \ PHY_ID_MATCH_MODEL(_id), \ @@ -831,6 +1095,9 @@ static int dp83822_resume(struct phy_device *phydev) .handle_interrupt = dp83822_handle_interrupt, \ .suspend = dp83822_suspend, \ .resume = dp83822_resume, \ + .led_hw_is_supported = dp83822_led_hw_is_supported, \ + .led_hw_control_set = dp83822_led_hw_control_set, \ + .led_hw_control_get = dp83822_led_hw_control_get, \ } #define DP83825_PHY_DRIVER(_id, _name) \
The DP83822 supports up to three configurable Light Emitting Diode (LED) pins: LED_0, LED_1 (GPIO1), COL (GPIO2) and RX_D3 (GPIO3). Several functions can be multiplexed onto the LEDs for different modes of operation. LED_0 and COL (GPIO2) use the MLED function. MLED can be routed to only one of these two pins at a time. Add minimal LED controller driver supporting the most common uses with the 'netdev' trigger. Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> --- drivers/net/phy/dp83822.c | 271 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 269 insertions(+), 2 deletions(-) --- base-commit: a14a429069bb1a18eb9fe63d68fcaa77dffe0e23 change-id: 20241215-dp83822-leds-bbdea724d726 Best regards,