diff mbox series

[net-next,v7,2/5] net: phy: mediatek: Move LED and read/write page helper functions into mtk phy lib

Message ID 20240613104023.13044-3-SkyLake.Huang@mediatek.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: phy: mediatek: Introduce mtk-phy-lib and add 2.5Gphy support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 856 this patch: 856
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 14 of 14 maintainers
netdev/build_clang success Errors and warnings before: 854 this patch: 854
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 861 this patch: 861
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 1070 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-06-16--18-00 (tests: 659)

Commit Message

SkyLake Huang (黃啟澤) June 13, 2024, 10:40 a.m. UTC 
From: "SkyLake.Huang" <skylake.huang@mediatek.com>

This patch moves mtk-ge-soc.c's LED code into mtk phy lib. We
can use those helper functions in mtk-ge.c as well. That is to
say, we have almost the same HW LED controller design in
mt7530/mt7531/mt7981/mt7988's Giga ethernet phy.

Also integrate read/write pages into one helper function. They
are basically the same.

Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com>
---
 MAINTAINERS                            |   2 +
 drivers/net/phy/mediatek/Kconfig       |   5 +
 drivers/net/phy/mediatek/Makefile      |   1 +
 drivers/net/phy/mediatek/mtk-ge-soc.c  | 340 ++++++-------------------
 drivers/net/phy/mediatek/mtk-ge.c      | 135 ++++++++--
 drivers/net/phy/mediatek/mtk-phy-lib.c | 247 ++++++++++++++++++
 drivers/net/phy/mediatek/mtk.h         |  83 ++++++
 7 files changed, 528 insertions(+), 285 deletions(-)
 create mode 100644 drivers/net/phy/mediatek/mtk-phy-lib.c
 create mode 100644 drivers/net/phy/mediatek/mtk.h

Comments

Simon Horman June 17, 2024, 11:51 a.m. UTC | #1 
On Thu, Jun 13, 2024 at 06:40:20PM +0800, Sky Huang wrote:
> From: "SkyLake.Huang" <skylake.huang@mediatek.com>
> 
> This patch moves mtk-ge-soc.c's LED code into mtk phy lib. We
> can use those helper functions in mtk-ge.c as well. That is to
> say, we have almost the same HW LED controller design in
> mt7530/mt7531/mt7981/mt7988's Giga ethernet phy.
> 
> Also integrate read/write pages into one helper function. They
> are basically the same.
> 
> Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com>

...

>  static int mt798x_phy_led_blink_set(struct phy_device *phydev, u8 index,
>  				    unsigned long *delay_on,
>  				    unsigned long *delay_off)
>  {
>  	bool blinking = false;
>  	int err = 0;
> +	struct mtk_socphy_priv *priv = phydev->priv;

Hi Sky,

A minor nit from my side.

If you need to respin this patchset for some other reason, please consider
preserving reverse xmas tree order - longest line to shortest - in this
function.

Likewise there are a few other changes in this patch which look like they
could be trivially updated to preserve or adopt reverse xmas tree order.

Edward Cree's tool can be of assistance here:
https://github.com/ecree-solarflare/xmastree

>  
>  	if (index > 1)
>  		return -EINVAL;
Jakub Kicinski June 18, 2024, 3:21 p.m. UTC | #2 
On Thu, 13 Jun 2024 18:40:20 +0800 Sky Huang wrote:
> This patch moves mtk-ge-soc.c's LED code into mtk phy lib. We
> can use those helper functions in mtk-ge.c as well. That is to
> say, we have almost the same HW LED controller design in
> mt7530/mt7531/mt7981/mt7988's Giga ethernet phy.
> 
> Also integrate read/write pages into one helper function. They
> are basically the same.

Could you please split this into multiple patches? maybe:
 - change the line wrapping
 - integrate read/write pages into one helper 
 - create mtk-phy-lib.c and mtk.h (pure code move)
 - add support for LEDs to the other SoC
SkyLake Huang (黃啟澤) June 19, 2024, 8:13 a.m. UTC | #3 
On Mon, 2024-06-17 at 12:51 +0100, Simon Horman wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Thu, Jun 13, 2024 at 06:40:20PM +0800, Sky Huang wrote:
> > From: "SkyLake.Huang" <skylake.huang@mediatek.com>
> > 
> > This patch moves mtk-ge-soc.c's LED code into mtk phy lib. We
> > can use those helper functions in mtk-ge.c as well. That is to
> > say, we have almost the same HW LED controller design in
> > mt7530/mt7531/mt7981/mt7988's Giga ethernet phy.
> > 
> > Also integrate read/write pages into one helper function. They
> > are basically the same.
> > 
> > Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com>
> 
> ...
> 
> >  static int mt798x_phy_led_blink_set(struct phy_device *phydev, u8
> index,
> >      unsigned long *delay_on,
> >      unsigned long *delay_off)
> >  {
> >  bool blinking = false;
> >  int err = 0;
> > +struct mtk_socphy_priv *priv = phydev->priv;
> 
> Hi Sky,
> 
> A minor nit from my side.
> 
> If you need to respin this patchset for some other reason, please
> consider
> preserving reverse xmas tree order - longest line to shortest - in
> this
> function.
> 
> Likewise there are a few other changes in this patch which look like
> they
> could be trivially updated to preserve or adopt reverse xmas tree
> order.
> 
> Edward Cree's tool can be of assistance here:
> https://github.com/ecree-solarflare/xmastree
> 
> >  
> >  if (index > 1)
> >  return -EINVAL;
Hi Simon,
  Thanks for reminding. There are some left-overs in mediatek-ge-soc.c
of current version on reverse Xmas tree order. I missed that. I'll
clean this up if we need to respin mtk-ge-soc.c again.

BRs,
Sky
SkyLake Huang (黃啟澤) June 19, 2024, 8:17 a.m. UTC | #4 
On Tue, 2024-06-18 at 08:21 -0700, Jakub Kicinski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Thu, 13 Jun 2024 18:40:20 +0800 Sky Huang wrote:
> > This patch moves mtk-ge-soc.c's LED code into mtk phy lib. We
> > can use those helper functions in mtk-ge.c as well. That is to
> > say, we have almost the same HW LED controller design in
> > mt7530/mt7531/mt7981/mt7988's Giga ethernet phy.
> > 
> > Also integrate read/write pages into one helper function. They
> > are basically the same.
> 
> Could you please split this into multiple patches? maybe:
>  - change the line wrapping
>  - integrate read/write pages into one helper 
>  - create mtk-phy-lib.c and mtk.h (pure code move)
>  - add support for LEDs to the other SoC

Hi Jakub,
  Mmm...Sorry. But is this really necessary? I already did similar
things in v2. IMHO, current patchset is small enough for reviewing. You
can easily tell that which parts come from original mediatek-ge.c &
mediatek-ge-soc.c and which part is used for MT7988's mtk-2p5ge.c

BRs,
Sky
Russell King (Oracle) June 19, 2024, 8:58 a.m. UTC | #5 
On Thu, Jun 13, 2024 at 06:40:20PM +0800, Sky Huang wrote:
> @@ -342,7 +295,8 @@ static int cal_cycle(struct phy_device *phydev, int devad,
>  	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1,
>  					MTK_PHY_RG_AD_CAL_CLK, reg_val,
>  					reg_val & MTK_PHY_DA_CAL_CLK, 500,
> -					ANALOG_INTERNAL_OPERATION_MAX_US, false);
> +					ANALOG_INTERNAL_OPERATION_MAX_US,
> +					false);

Again, this patch is moving code around. There should be no extraneous
changes in such a patch - it should just move code around, fixing stuff
up for the code move. Any other change (such as reformatting) should be
done as a separate patch.

>  	if (ret) {
>  		phydev_err(phydev, "Calibration cycle timeout\n");
>  		return ret;
> @@ -351,7 +305,7 @@ static int cal_cycle(struct phy_device *phydev, int devad,
>  	phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_RG_AD_CALIN,
>  			   MTK_PHY_DA_CALIN_FLAG);
>  	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_RG_AD_CAL_COMP) >>
> -			   MTK_PHY_AD_CAL_COMP_OUT_SHIFT;
> +	      MTK_PHY_AD_CAL_COMP_OUT_SHIFT;

I don't see a reason for this change, and it goes against established
coding style.

>  	phydev_dbg(phydev, "cal_val: 0x%x, ret: %d\n", cal_val, ret);
>  
>  	return ret;
> @@ -440,38 +394,46 @@ static int tx_amp_fill_result(struct phy_device *phydev, u16 *buf)
>  	}
>  
>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TXVLD_DA_RG,
> -		       MTK_PHY_DA_TX_I2MPB_A_GBE_MASK, (buf[0] + bias[0]) << 10);
> +		       MTK_PHY_DA_TX_I2MPB_A_GBE_MASK,
> +		       (buf[0] + bias[0]) << 10);

Likewise, this is an extraneous change to the primary purpose of this
patch which is to reorganise code to a new file. 

>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TXVLD_DA_RG,
>  		       MTK_PHY_DA_TX_I2MPB_A_TBT_MASK, buf[0] + bias[1]);
>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_A2,
> -		       MTK_PHY_DA_TX_I2MPB_A_HBT_MASK, (buf[0] + bias[2]) << 10);
> +		       MTK_PHY_DA_TX_I2MPB_A_HBT_MASK,
> +		       (buf[0] + bias[2]) << 10);

Same again.

>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_A2,
>  		       MTK_PHY_DA_TX_I2MPB_A_TST_MASK, buf[0] + bias[3]);
>  
>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_B1,
> -		       MTK_PHY_DA_TX_I2MPB_B_GBE_MASK, (buf[1] + bias[4]) << 8);
> +		       MTK_PHY_DA_TX_I2MPB_B_GBE_MASK,
> +		       (buf[1] + bias[4]) << 8);

And here.

>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_B1,
>  		       MTK_PHY_DA_TX_I2MPB_B_TBT_MASK, buf[1] + bias[5]);
>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_B2,
> -		       MTK_PHY_DA_TX_I2MPB_B_HBT_MASK, (buf[1] + bias[6]) << 8);
> +		       MTK_PHY_DA_TX_I2MPB_B_HBT_MASK,
> +		       (buf[1] + bias[6]) << 8);

And here.

>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_B2,
>  		       MTK_PHY_DA_TX_I2MPB_B_TST_MASK, buf[1] + bias[7]);
>  
>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_C1,
> -		       MTK_PHY_DA_TX_I2MPB_C_GBE_MASK, (buf[2] + bias[8]) << 8);
> +		       MTK_PHY_DA_TX_I2MPB_C_GBE_MASK,
> +		       (buf[2] + bias[8]) << 8);

Ditto.

>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_C1,
>  		       MTK_PHY_DA_TX_I2MPB_C_TBT_MASK, buf[2] + bias[9]);
>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_C2,
> -		       MTK_PHY_DA_TX_I2MPB_C_HBT_MASK, (buf[2] + bias[10]) << 8);
> +		       MTK_PHY_DA_TX_I2MPB_C_HBT_MASK,
> +		       (buf[2] + bias[10]) << 8);

Ditto.

>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_C2,
>  		       MTK_PHY_DA_TX_I2MPB_C_TST_MASK, buf[2] + bias[11]);
>  
>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_D1,
> -		       MTK_PHY_DA_TX_I2MPB_D_GBE_MASK, (buf[3] + bias[12]) << 8);
> +		       MTK_PHY_DA_TX_I2MPB_D_GBE_MASK,
> +		       (buf[3] + bias[12]) << 8);

Ditto.

>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_D1,
>  		       MTK_PHY_DA_TX_I2MPB_D_TBT_MASK, buf[3] + bias[13]);
>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_D2,
> -		       MTK_PHY_DA_TX_I2MPB_D_HBT_MASK, (buf[3] + bias[14]) << 8);
> +		       MTK_PHY_DA_TX_I2MPB_D_HBT_MASK,
> +		       (buf[3] + bias[14]) << 8);

Ditto.

>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_D2,
>  		       MTK_PHY_DA_TX_I2MPB_D_TST_MASK, buf[3] + bias[15]);
>  
> @@ -662,7 +624,8 @@ static int tx_vcm_cal_sw(struct phy_device *phydev, u8 rg_txreserve_x)
>  		goto restore;
>  
>  	/* We calibrate TX-VCM in different logic. Check upper index and then
> -	 * lower index. If this calibration is valid, apply lower index's result.
> +	 * lower index. If this calibration is valid, apply lower index's
> +	 * result.

Ditto.

>  	 */
>  	ret = upper_ret - lower_ret;
>  	if (ret == 1) {
> @@ -691,7 +654,8 @@ static int tx_vcm_cal_sw(struct phy_device *phydev, u8 rg_txreserve_x)
>  	} else if (upper_idx == TXRESERVE_MAX && upper_ret == 0 &&
>  		   lower_ret == 0) {
>  		ret = 0;
> -		phydev_warn(phydev, "TX-VCM SW cal result at high margin 0x%x\n",
> +		phydev_warn(phydev,
> +			    "TX-VCM SW cal result at high margin 0x%x\n",

Ditto.

>  			    upper_idx);
>  	} else {
>  		ret = -EINVAL;
> @@ -795,7 +759,8 @@ static void mt7981_phy_finetune(struct phy_device *phydev)
>  
>  	/* TR_OPEN_LOOP_EN = 1, lpf_x_average = 9 */
>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_RG_DEV1E_REG234,
> -		       MTK_PHY_TR_OPEN_LOOP_EN_MASK | MTK_PHY_LPF_X_AVERAGE_MASK,
> +		       MTK_PHY_TR_OPEN_LOOP_EN_MASK |
> +		       MTK_PHY_LPF_X_AVERAGE_MASK,

Ditto.

>  		       BIT(0) | FIELD_PREP(MTK_PHY_LPF_X_AVERAGE_MASK, 0x9));
>  
>  	/* rg_tr_lpf_cnt_val = 512 */
> @@ -864,7 +829,8 @@ static void mt7988_phy_finetune(struct phy_device *phydev)
>  
>  	/* TR_OPEN_LOOP_EN = 1, lpf_x_average = 10 */
>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_RG_DEV1E_REG234,
> -		       MTK_PHY_TR_OPEN_LOOP_EN_MASK | MTK_PHY_LPF_X_AVERAGE_MASK,
> +		       MTK_PHY_TR_OPEN_LOOP_EN_MASK |
> +		       MTK_PHY_LPF_X_AVERAGE_MASK,

Ditto.

>  		       BIT(0) | FIELD_PREP(MTK_PHY_LPF_X_AVERAGE_MASK, 0xa));
>  
>  	/* rg_tr_lpf_cnt_val = 1023 */
> @@ -976,7 +942,8 @@ static void mt798x_phy_eee(struct phy_device *phydev)
>  	phy_restore_page(phydev, MTK_PHY_PAGE_STANDARD, 0);
>  
>  	phy_select_page(phydev, MTK_PHY_PAGE_EXTENDED_3);
> -	__phy_modify(phydev, MTK_PHY_LPI_REG_14, MTK_PHY_LPI_WAKE_TIMER_1000_MASK,
> +	__phy_modify(phydev, MTK_PHY_LPI_REG_14,
> +		     MTK_PHY_LPI_WAKE_TIMER_1000_MASK,
>  		     FIELD_PREP(MTK_PHY_LPI_WAKE_TIMER_1000_MASK, 0x19c));

Ditto.

>  
>  	__phy_modify(phydev, MTK_PHY_LPI_REG_1c, MTK_PHY_SMI_DET_ON_THRESH_MASK,
> @@ -986,7 +953,8 @@ static void mt798x_phy_eee(struct phy_device *phydev)
>  	phy_modify_mmd(phydev, MDIO_MMD_VEND1,
>  		       MTK_PHY_RG_LPI_PCS_DSP_CTRL_REG122,
>  		       MTK_PHY_LPI_NORM_MSE_HI_THRESH1000_MASK,
> -		       FIELD_PREP(MTK_PHY_LPI_NORM_MSE_HI_THRESH1000_MASK, 0xff));
> +		       FIELD_PREP(MTK_PHY_LPI_NORM_MSE_HI_THRESH1000_MASK,
> +				  0xff));

Ditto.

>  }
>  
>  static int cal_sw(struct phy_device *phydev, enum CAL_ITEM cal_item,
> @@ -1130,57 +1098,13 @@ static int mt798x_phy_config_init(struct phy_device *phydev)
>  	return mt798x_phy_calibration(phydev);
>  }
>  
> -static int mt798x_phy_hw_led_on_set(struct phy_device *phydev, u8 index,
> -				    bool on)
> -{
> -	unsigned int bit_on = MTK_PHY_LED_STATE_FORCE_ON + (index ? 16 : 0);
> -	struct mtk_socphy_priv *priv = phydev->priv;
> -	bool changed;
> -
> -	if (on)
> -		changed = !test_and_set_bit(bit_on, &priv->led_state);
> -	else
> -		changed = !!test_and_clear_bit(bit_on, &priv->led_state);
> -
> -	changed |= !!test_and_clear_bit(MTK_PHY_LED_STATE_NETDEV +
> -					(index ? 16 : 0), &priv->led_state);
> -	if (changed)
> -		return phy_modify_mmd(phydev, MDIO_MMD_VEND2, index ?
> -				      MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL,

If you're moving this code, don't even correct this formatting in the
new file. If you correct the formatting in a patch _before_ this one,
then do the move, the formatting will then be correct in the new file
and the code move will be exactly that - just a code move.

>  static int mt798x_phy_led_blink_set(struct phy_device *phydev, u8 index,
>  				    unsigned long *delay_on,
>  				    unsigned long *delay_off)
>  {
>  	bool blinking = false;
>  	int err = 0;
> +	struct mtk_socphy_priv *priv = phydev->priv;

Netdev wants reverse Christmas-tree. Longest local variable declaration
first, followed by next shorter and so on. So, in this case, it should
be priv, blinking, then err in that order.

>  static int mt798x_phy_led_brightness_set(struct phy_device *phydev,
>  					 u8 index, enum led_brightness value)
>  {
>  	int err;
> +	struct mtk_socphy_priv *priv = phydev->priv;

Reverse Christmas tree please.

>  static int mt798x_phy_led_hw_control_set(struct phy_device *phydev, u8 index,
>  					 unsigned long rules)
>  {
> -	unsigned int bit_netdev = MTK_PHY_LED_STATE_NETDEV + (index ? 16 : 0);
>  	struct mtk_socphy_priv *priv = phydev->priv;
> -	u16 on = 0, blink = 0;
> -	int ret;
> -
> -	if (index > 1)
> -		return -EINVAL;
> -
> -	if (rules & BIT(TRIGGER_NETDEV_FULL_DUPLEX))
> -		on |= MTK_PHY_LED_ON_FDX;
> -
> -	if (rules & BIT(TRIGGER_NETDEV_HALF_DUPLEX))
> -		on |= MTK_PHY_LED_ON_HDX;
> -
> -	if (rules & (BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK)))
> -		on |= MTK_PHY_LED_ON_LINK10;
> -
> -	if (rules & (BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK)))
> -		on |= MTK_PHY_LED_ON_LINK100;
> -
> -	if (rules & (BIT(TRIGGER_NETDEV_LINK_1000) | BIT(TRIGGER_NETDEV_LINK)))
> -		on |= MTK_PHY_LED_ON_LINK1000;
>  
> -	if (rules & BIT(TRIGGER_NETDEV_RX)) {
> -		blink |= (on & MTK_PHY_LED_ON_LINK) ?
> -			  (((on & MTK_PHY_LED_ON_LINK10) ? MTK_PHY_LED_BLINK_10RX : 0) |
> -			   ((on & MTK_PHY_LED_ON_LINK100) ? MTK_PHY_LED_BLINK_100RX : 0) |
> -			   ((on & MTK_PHY_LED_ON_LINK1000) ? MTK_PHY_LED_BLINK_1000RX : 0)) :
> -			  MTK_PHY_LED_BLINK_RX;

Now for this mess - and it should've been caught earlier... far too many
parens. Too many parens make code unreadable. Also, we know "blink"
starts off with the bits clear. So:

	if (rules & BIT(TRIGGER_NETDEV_RX)) {
		if (on & MTK_PHY_LED_ON_LINK) {
			if (on & MTK_PHY_LED_ON_LINK10)
				blink |= MTK_PHY_LED_BLINK_10RX;
			if (on & MTK_PHY_LED_ON_LINK100)
				blink |= MTK_PHY_LED_BLINK_100RX;
			if (on & MTK_PHY_LED_ON_LINK1000)
				blink |= MTK_PHY_LED_BLINK_1000RX;
		} else {
			blink |= MTK_PHY_LED_BLINK_RX;
		}
	}

is a much nicer and easier to read formatting.

Thanks.
SkyLake Huang (黃啟澤) June 19, 2024, 10:37 a.m. UTC | #6 
On Wed, 2024-06-19 at 16:17 +0800, SkyLake.Huang wrote:
> On Tue, 2024-06-18 at 08:21 -0700, Jakub Kicinski wrote:
> >  	 
> > External email : Please do not click links or open attachments
> > until
> > you have verified the sender or the content.
> >  On Thu, 13 Jun 2024 18:40:20 +0800 Sky Huang wrote:
> > > This patch moves mtk-ge-soc.c's LED code into mtk phy lib. We
> > > can use those helper functions in mtk-ge.c as well. That is to
> > > say, we have almost the same HW LED controller design in
> > > mt7530/mt7531/mt7981/mt7988's Giga ethernet phy.
> > > 
> > > Also integrate read/write pages into one helper function. They
> > > are basically the same.
> > 
> > Could you please split this into multiple patches? maybe:
> >  - change the line wrapping
> >  - integrate read/write pages into one helper 
> >  - create mtk-phy-lib.c and mtk.h (pure code move)
> >  - add support for LEDs to the other SoC
> 
> Hi Jakub,
>   Mmm...Sorry. But is this really necessary? I already did similar
> things in v2. IMHO, current patchset is small enough for reviewing.
> You
> can easily tell that which parts come from original mediatek-ge.c &
> mediatek-ge-soc.c and which part is used for MT7988's mtk-2p5ge.c
> 
> BRs,
> Sky

Hi Jakub,
  After seeing Russell's comments and reviewing my patch again, I think
I got your points. I'll re-organize this patch in this way:
- create mtk-phy-lib.c & mtk.h and move LED helpers from mtk-ge-soc.c
- integrate read/write pages into one helper
- hook LED helper functions in mtk-ge.c
- Add MT7530 & MT7531's PHY ID macros
- Shrink mtk-ge-soc.c line wrapping to 80 characters

BRs,
Sky
SkyLake Huang (黃啟澤) June 19, 2024, 11:21 a.m. UTC | #7 
On Wed, 2024-06-19 at 09:58 +0100, Russell King (Oracle) wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Thu, Jun 13, 2024 at 06:40:20PM +0800, Sky Huang wrote:
> > @@ -342,7 +295,8 @@ static int cal_cycle(struct phy_device *phydev,
> int devad,
> >  ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1,
> >  MTK_PHY_RG_AD_CAL_CLK, reg_val,
> >  reg_val & MTK_PHY_DA_CAL_CLK, 500,
> > -ANALOG_INTERNAL_OPERATION_MAX_US, false);
> > +ANALOG_INTERNAL_OPERATION_MAX_US,
> > +false);
> 
> Again, this patch is moving code around. There should be no
> extraneous
> changes in such a patch - it should just move code around, fixing
> stuff
> up for the code move. Any other change (such as reformatting) should
> be
> done as a separate patch.
> 
> >  if (ret) {
> >  phydev_err(phydev, "Calibration cycle timeout\n");
> >  return ret;
> > @@ -351,7 +305,7 @@ static int cal_cycle(struct phy_device *phydev,
> int devad,
> >  phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_RG_AD_CALIN,
> >     MTK_PHY_DA_CALIN_FLAG);
> >  ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_RG_AD_CAL_COMP)
> >>
> > -   MTK_PHY_AD_CAL_COMP_OUT_SHIFT;
> > +      MTK_PHY_AD_CAL_COMP_OUT_SHIFT;
> 
> I don't see a reason for this change, and it goes against established
> coding style.
> 
> >  phydev_dbg(phydev, "cal_val: 0x%x, ret: %d\n", cal_val, ret);
> >  
> >  return ret;
> > @@ -440,38 +394,46 @@ static int tx_amp_fill_result(struct
> phy_device *phydev, u16 *buf)
> >  }
> >  
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TXVLD_DA_RG,
> > -       MTK_PHY_DA_TX_I2MPB_A_GBE_MASK, (buf[0] + bias[0]) << 10);
> > +       MTK_PHY_DA_TX_I2MPB_A_GBE_MASK,
> > +       (buf[0] + bias[0]) << 10);
> 
> Likewise, this is an extraneous change to the primary purpose of this
> patch which is to reorganise code to a new file. 
> 
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TXVLD_DA_RG,
> >         MTK_PHY_DA_TX_I2MPB_A_TBT_MASK, buf[0] + bias[1]);
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> MTK_PHY_TX_I2MPB_TEST_MODE_A2,
> > -       MTK_PHY_DA_TX_I2MPB_A_HBT_MASK, (buf[0] + bias[2]) << 10);
> > +       MTK_PHY_DA_TX_I2MPB_A_HBT_MASK,
> > +       (buf[0] + bias[2]) << 10);
> 
> Same again.
> 
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> MTK_PHY_TX_I2MPB_TEST_MODE_A2,
> >         MTK_PHY_DA_TX_I2MPB_A_TST_MASK, buf[0] + bias[3]);
> >  
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> MTK_PHY_TX_I2MPB_TEST_MODE_B1,
> > -       MTK_PHY_DA_TX_I2MPB_B_GBE_MASK, (buf[1] + bias[4]) << 8);
> > +       MTK_PHY_DA_TX_I2MPB_B_GBE_MASK,
> > +       (buf[1] + bias[4]) << 8);
> 
> And here.
> 
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> MTK_PHY_TX_I2MPB_TEST_MODE_B1,
> >         MTK_PHY_DA_TX_I2MPB_B_TBT_MASK, buf[1] + bias[5]);
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> MTK_PHY_TX_I2MPB_TEST_MODE_B2,
> > -       MTK_PHY_DA_TX_I2MPB_B_HBT_MASK, (buf[1] + bias[6]) << 8);
> > +       MTK_PHY_DA_TX_I2MPB_B_HBT_MASK,
> > +       (buf[1] + bias[6]) << 8);
> 
> And here.
> 
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> MTK_PHY_TX_I2MPB_TEST_MODE_B2,
> >         MTK_PHY_DA_TX_I2MPB_B_TST_MASK, buf[1] + bias[7]);
> >  
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> MTK_PHY_TX_I2MPB_TEST_MODE_C1,
> > -       MTK_PHY_DA_TX_I2MPB_C_GBE_MASK, (buf[2] + bias[8]) << 8);
> > +       MTK_PHY_DA_TX_I2MPB_C_GBE_MASK,
> > +       (buf[2] + bias[8]) << 8);
> 
> Ditto.
> 
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> MTK_PHY_TX_I2MPB_TEST_MODE_C1,
> >         MTK_PHY_DA_TX_I2MPB_C_TBT_MASK, buf[2] + bias[9]);
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> MTK_PHY_TX_I2MPB_TEST_MODE_C2,
> > -       MTK_PHY_DA_TX_I2MPB_C_HBT_MASK, (buf[2] + bias[10]) << 8);
> > +       MTK_PHY_DA_TX_I2MPB_C_HBT_MASK,
> > +       (buf[2] + bias[10]) << 8);
> 
> Ditto.
> 
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> MTK_PHY_TX_I2MPB_TEST_MODE_C2,
> >         MTK_PHY_DA_TX_I2MPB_C_TST_MASK, buf[2] + bias[11]);
> >  
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> MTK_PHY_TX_I2MPB_TEST_MODE_D1,
> > -       MTK_PHY_DA_TX_I2MPB_D_GBE_MASK, (buf[3] + bias[12]) << 8);
> > +       MTK_PHY_DA_TX_I2MPB_D_GBE_MASK,
> > +       (buf[3] + bias[12]) << 8);
> 
> Ditto.
> 
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> MTK_PHY_TX_I2MPB_TEST_MODE_D1,
> >         MTK_PHY_DA_TX_I2MPB_D_TBT_MASK, buf[3] + bias[13]);
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> MTK_PHY_TX_I2MPB_TEST_MODE_D2,
> > -       MTK_PHY_DA_TX_I2MPB_D_HBT_MASK, (buf[3] + bias[14]) << 8);
> > +       MTK_PHY_DA_TX_I2MPB_D_HBT_MASK,
> > +       (buf[3] + bias[14]) << 8);
> 
> Ditto.
> 
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> MTK_PHY_TX_I2MPB_TEST_MODE_D2,
> >         MTK_PHY_DA_TX_I2MPB_D_TST_MASK, buf[3] + bias[15]);
> >  
> > @@ -662,7 +624,8 @@ static int tx_vcm_cal_sw(struct phy_device
> *phydev, u8 rg_txreserve_x)
> >  goto restore;
> >  
> >  /* We calibrate TX-VCM in different logic. Check upper index and
> then
> > - * lower index. If this calibration is valid, apply lower index's
> result.
> > + * lower index. If this calibration is valid, apply lower index's
> > + * result.
> 
> Ditto.
> 
> >   */
> >  ret = upper_ret - lower_ret;
> >  if (ret == 1) {
> > @@ -691,7 +654,8 @@ static int tx_vcm_cal_sw(struct phy_device
> *phydev, u8 rg_txreserve_x)
> >  } else if (upper_idx == TXRESERVE_MAX && upper_ret == 0 &&
> >     lower_ret == 0) {
> >  ret = 0;
> > -phydev_warn(phydev, "TX-VCM SW cal result at high margin 0x%x\n",
> > +phydev_warn(phydev,
> > +    "TX-VCM SW cal result at high margin 0x%x\n",
> 
> Ditto.
> 
> >      upper_idx);
> >  } else {
> >  ret = -EINVAL;
> > @@ -795,7 +759,8 @@ static void mt7981_phy_finetune(struct
> phy_device *phydev)
> >  
> >  /* TR_OPEN_LOOP_EN = 1, lpf_x_average = 9 */
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_RG_DEV1E_REG234,
> > -       MTK_PHY_TR_OPEN_LOOP_EN_MASK | MTK_PHY_LPF_X_AVERAGE_MASK,
> > +       MTK_PHY_TR_OPEN_LOOP_EN_MASK |
> > +       MTK_PHY_LPF_X_AVERAGE_MASK,
> 
> Ditto.
> 
> >         BIT(0) | FIELD_PREP(MTK_PHY_LPF_X_AVERAGE_MASK, 0x9));
> >  
> >  /* rg_tr_lpf_cnt_val = 512 */
> > @@ -864,7 +829,8 @@ static void mt7988_phy_finetune(struct
> phy_device *phydev)
> >  
> >  /* TR_OPEN_LOOP_EN = 1, lpf_x_average = 10 */
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_RG_DEV1E_REG234,
> > -       MTK_PHY_TR_OPEN_LOOP_EN_MASK | MTK_PHY_LPF_X_AVERAGE_MASK,
> > +       MTK_PHY_TR_OPEN_LOOP_EN_MASK |
> > +       MTK_PHY_LPF_X_AVERAGE_MASK,
> 
> Ditto.
> 
> >         BIT(0) | FIELD_PREP(MTK_PHY_LPF_X_AVERAGE_MASK, 0xa));
> >  
> >  /* rg_tr_lpf_cnt_val = 1023 */
> > @@ -976,7 +942,8 @@ static void mt798x_phy_eee(struct phy_device
> *phydev)
> >  phy_restore_page(phydev, MTK_PHY_PAGE_STANDARD, 0);
> >  
> >  phy_select_page(phydev, MTK_PHY_PAGE_EXTENDED_3);
> > -__phy_modify(phydev, MTK_PHY_LPI_REG_14,
> MTK_PHY_LPI_WAKE_TIMER_1000_MASK,
> > +__phy_modify(phydev, MTK_PHY_LPI_REG_14,
> > +     MTK_PHY_LPI_WAKE_TIMER_1000_MASK,
> >       FIELD_PREP(MTK_PHY_LPI_WAKE_TIMER_1000_MASK, 0x19c));
> 
> Ditto.
> 
> >  
> >  __phy_modify(phydev, MTK_PHY_LPI_REG_1c,
> MTK_PHY_SMI_DET_ON_THRESH_MASK,
> > @@ -986,7 +953,8 @@ static void mt798x_phy_eee(struct phy_device
> *phydev)
> >  phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> >         MTK_PHY_RG_LPI_PCS_DSP_CTRL_REG122,
> >         MTK_PHY_LPI_NORM_MSE_HI_THRESH1000_MASK,
> > -       FIELD_PREP(MTK_PHY_LPI_NORM_MSE_HI_THRESH1000_MASK, 0xff));
> > +       FIELD_PREP(MTK_PHY_LPI_NORM_MSE_HI_THRESH1000_MASK,
> > +  0xff));
> 
> Ditto.
> 
> >  }
> >  
> >  static int cal_sw(struct phy_device *phydev, enum CAL_ITEM
> cal_item,
> > @@ -1130,57 +1098,13 @@ static int mt798x_phy_config_init(struct
> phy_device *phydev)
> >  return mt798x_phy_calibration(phydev);
> >  }
> >  
> > -static int mt798x_phy_hw_led_on_set(struct phy_device *phydev, u8
> index,
> > -    bool on)
> > -{
> > -unsigned int bit_on = MTK_PHY_LED_STATE_FORCE_ON + (index ? 16 :
> 0);
> > -struct mtk_socphy_priv *priv = phydev->priv;
> > -bool changed;
> > -
> > -if (on)
> > -changed = !test_and_set_bit(bit_on, &priv->led_state);
> > -else
> > -changed = !!test_and_clear_bit(bit_on, &priv->led_state);
> > -
> > -changed |= !!test_and_clear_bit(MTK_PHY_LED_STATE_NETDEV +
> > -(index ? 16 : 0), &priv->led_state);
> > -if (changed)
> > -return phy_modify_mmd(phydev, MDIO_MMD_VEND2, index ?
> > -      MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL,
> 
> If you're moving this code, don't even correct this formatting in the
> new file. If you correct the formatting in a patch _before_ this one,
> then do the move, the formatting will then be correct in the new file
> and the code move will be exactly that - just a code move.
> 
> >  static int mt798x_phy_led_blink_set(struct phy_device *phydev, u8
> index,
> >      unsigned long *delay_on,
> >      unsigned long *delay_off)
> >  {
> >  bool blinking = false;
> >  int err = 0;
> > +struct mtk_socphy_priv *priv = phydev->priv;
> 
> Netdev wants reverse Christmas-tree. Longest local variable
> declaration
> first, followed by next shorter and so on. So, in this case, it
> should
> be priv, blinking, then err in that order.
> 
> >  static int mt798x_phy_led_brightness_set(struct phy_device
> *phydev,
> >   u8 index, enum led_brightness value)
> >  {
> >  int err;
> > +struct mtk_socphy_priv *priv = phydev->priv;
> 
> Reverse Christmas tree please.
> 
I'll add a few other patches to seperately show "formatting fixing" and
"reverse Xmas tree fixing".

> >  static int mt798x_phy_led_hw_control_set(struct phy_device
> *phydev, u8 index,
> >   unsigned long rules)
> >  {
> > -unsigned int bit_netdev = MTK_PHY_LED_STATE_NETDEV + (index ? 16 :
> 0);
> >  struct mtk_socphy_priv *priv = phydev->priv;
> > -u16 on = 0, blink = 0;
> > -int ret;
> > -
> > -if (index > 1)
> > -return -EINVAL;
> > -
> > -if (rules & BIT(TRIGGER_NETDEV_FULL_DUPLEX))
> > -on |= MTK_PHY_LED_ON_FDX;
> > -
> > -if (rules & BIT(TRIGGER_NETDEV_HALF_DUPLEX))
> > -on |= MTK_PHY_LED_ON_HDX;
> > -
> > -if (rules & (BIT(TRIGGER_NETDEV_LINK_10) |
> BIT(TRIGGER_NETDEV_LINK)))
> > -on |= MTK_PHY_LED_ON_LINK10;
> > -
> > -if (rules & (BIT(TRIGGER_NETDEV_LINK_100) |
> BIT(TRIGGER_NETDEV_LINK)))
> > -on |= MTK_PHY_LED_ON_LINK100;
> > -
> > -if (rules & (BIT(TRIGGER_NETDEV_LINK_1000) |
> BIT(TRIGGER_NETDEV_LINK)))
> > -on |= MTK_PHY_LED_ON_LINK1000;
> >  
> > -if (rules & BIT(TRIGGER_NETDEV_RX)) {
> > -blink |= (on & MTK_PHY_LED_ON_LINK) ?
> > -  (((on & MTK_PHY_LED_ON_LINK10) ? MTK_PHY_LED_BLINK_10RX : 0) |
> > -   ((on & MTK_PHY_LED_ON_LINK100) ? MTK_PHY_LED_BLINK_100RX : 0) |
> > -   ((on & MTK_PHY_LED_ON_LINK1000) ? MTK_PHY_LED_BLINK_1000RX :
> 0)) :
> > -  MTK_PHY_LED_BLINK_RX;
> 
> Now for this mess - and it should've been caught earlier... far too
> many
> parens. Too many parens make code unreadable. Also, we know "blink"
> starts off with the bits clear. So:
> 
> if (rules & BIT(TRIGGER_NETDEV_RX)) {
> if (on & MTK_PHY_LED_ON_LINK) {
> if (on & MTK_PHY_LED_ON_LINK10)
> blink |= MTK_PHY_LED_BLINK_10RX;
> if (on & MTK_PHY_LED_ON_LINK100)
> blink |= MTK_PHY_LED_BLINK_100RX;
> if (on & MTK_PHY_LED_ON_LINK1000)
> blink |= MTK_PHY_LED_BLINK_1000RX;
> } else {
> blink |= MTK_PHY_LED_BLINK_RX;
> }
> }
> 
> is a much nicer and easier to read formatting.
> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Thanks. I'll fix this in next version.

BRs,
Sky
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 6deaf94..e58e05c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13794,7 +13794,9 @@  M:	SkyLake Huang <SkyLake.Huang@mediatek.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/phy/mediatek/mtk-ge-soc.c
+F:	drivers/net/phy/mediatek/mtk-phy-lib.c
 F:	drivers/net/phy/mediatek/mtk-ge.c
+F:	drivers/net/phy/mediatek/mtk.h
 F:	drivers/phy/mediatek/phy-mtk-xfi-tphy.c
 
 MEDIATEK I2C CONTROLLER DRIVER
diff --git a/drivers/net/phy/mediatek/Kconfig b/drivers/net/phy/mediatek/Kconfig
index 6839ea6..448bc20 100644
--- a/drivers/net/phy/mediatek/Kconfig
+++ b/drivers/net/phy/mediatek/Kconfig
@@ -1,6 +1,10 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
+config MTK_NET_PHYLIB
+	tristate
+
 config MEDIATEK_GE_PHY
 	tristate "MediaTek Gigabit Ethernet PHYs"
+	select MTK_NET_PHYLIB
 	help
 	  Supports the MediaTek non-built-in Gigabit Ethernet PHYs.
 
@@ -13,6 +17,7 @@  config MEDIATEK_GE_SOC_PHY
 	tristate "MediaTek SoC Ethernet PHYs"
 	depends on (ARM64 && ARCH_MEDIATEK) || COMPILE_TEST
 	select NVMEM_MTK_EFUSE
+	select MTK_NET_PHYLIB
 	help
 	  Supports MediaTek SoC built-in Gigabit Ethernet PHYs.
 
diff --git a/drivers/net/phy/mediatek/Makefile b/drivers/net/phy/mediatek/Makefile
index 005bde2..814879d 100644
--- a/drivers/net/phy/mediatek/Makefile
+++ b/drivers/net/phy/mediatek/Makefile
@@ -1,3 +1,4 @@ 
 # SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_MTK_NET_PHYLIB)		+= mtk-phy-lib.o
 obj-$(CONFIG_MEDIATEK_GE_PHY)		+= mtk-ge.o
 obj-$(CONFIG_MEDIATEK_GE_SOC_PHY)	+= mtk-ge-soc.o
diff --git a/drivers/net/phy/mediatek/mtk-ge-soc.c b/drivers/net/phy/mediatek/mtk-ge-soc.c
index 47af872..fa65e61 100644
--- a/drivers/net/phy/mediatek/mtk-ge-soc.c
+++ b/drivers/net/phy/mediatek/mtk-ge-soc.c
@@ -8,6 +8,8 @@ 
 #include <linux/phy.h>
 #include <linux/regmap.h>
 
+#include "mtk.h"
+
 #define MTK_GPHY_ID_MT7981			0x03a29461
 #define MTK_GPHY_ID_MT7988			0x03a29481
 
@@ -210,41 +212,6 @@ 
 #define MTK_PHY_DA_TX_R50_PAIR_D		0x540
 
 /* Registers on MDIO_MMD_VEND2 */
-#define MTK_PHY_LED0_ON_CTRL			0x24
-#define MTK_PHY_LED1_ON_CTRL			0x26
-#define   MTK_PHY_LED_ON_MASK			GENMASK(6, 0)
-#define   MTK_PHY_LED_ON_LINK1000		BIT(0)
-#define   MTK_PHY_LED_ON_LINK100		BIT(1)
-#define   MTK_PHY_LED_ON_LINK10			BIT(2)
-#define   MTK_PHY_LED_ON_LINK			(MTK_PHY_LED_ON_LINK10 |\
-						 MTK_PHY_LED_ON_LINK100 |\
-						 MTK_PHY_LED_ON_LINK1000)
-#define   MTK_PHY_LED_ON_LINKDOWN		BIT(3)
-#define   MTK_PHY_LED_ON_FDX			BIT(4) /* Full duplex */
-#define   MTK_PHY_LED_ON_HDX			BIT(5) /* Half duplex */
-#define   MTK_PHY_LED_ON_FORCE_ON		BIT(6)
-#define   MTK_PHY_LED_ON_POLARITY		BIT(14)
-#define   MTK_PHY_LED_ON_ENABLE			BIT(15)
-
-#define MTK_PHY_LED0_BLINK_CTRL			0x25
-#define MTK_PHY_LED1_BLINK_CTRL			0x27
-#define   MTK_PHY_LED_BLINK_1000TX		BIT(0)
-#define   MTK_PHY_LED_BLINK_1000RX		BIT(1)
-#define   MTK_PHY_LED_BLINK_100TX		BIT(2)
-#define   MTK_PHY_LED_BLINK_100RX		BIT(3)
-#define   MTK_PHY_LED_BLINK_10TX		BIT(4)
-#define   MTK_PHY_LED_BLINK_10RX		BIT(5)
-#define   MTK_PHY_LED_BLINK_RX			(MTK_PHY_LED_BLINK_10RX |\
-						 MTK_PHY_LED_BLINK_100RX |\
-						 MTK_PHY_LED_BLINK_1000RX)
-#define   MTK_PHY_LED_BLINK_TX			(MTK_PHY_LED_BLINK_10TX |\
-						 MTK_PHY_LED_BLINK_100TX |\
-						 MTK_PHY_LED_BLINK_1000TX)
-#define   MTK_PHY_LED_BLINK_COLLISION		BIT(6)
-#define   MTK_PHY_LED_BLINK_RX_CRC_ERR		BIT(7)
-#define   MTK_PHY_LED_BLINK_RX_IDLE_ERR		BIT(8)
-#define   MTK_PHY_LED_BLINK_FORCE_BLINK		BIT(9)
-
 #define MTK_PHY_LED1_DEFAULT_POLARITIES		BIT(1)
 
 #define MTK_PHY_RG_BG_RASEL			0x115
@@ -299,10 +266,6 @@  enum CAL_MODE {
 	SW_M
 };
 
-#define MTK_PHY_LED_STATE_FORCE_ON	0
-#define MTK_PHY_LED_STATE_FORCE_BLINK	1
-#define MTK_PHY_LED_STATE_NETDEV	2
-
 struct mtk_socphy_priv {
 	unsigned long		led_state;
 };
@@ -312,16 +275,6 @@  struct mtk_socphy_shared {
 	struct mtk_socphy_priv	priv[4];
 };
 
-static int mtk_socphy_read_page(struct phy_device *phydev)
-{
-	return __phy_read(phydev, MTK_EXT_PAGE_ACCESS);
-}
-
-static int mtk_socphy_write_page(struct phy_device *phydev, int page)
-{
-	return __phy_write(phydev, MTK_EXT_PAGE_ACCESS, page);
-}
-
 /* One calibration cycle consists of:
  * 1.Set DA_CALIN_FLAG high to start calibration. Keep it high
  *   until AD_CAL_COMP is ready to output calibration result.
@@ -342,7 +295,8 @@  static int cal_cycle(struct phy_device *phydev, int devad,
 	ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1,
 					MTK_PHY_RG_AD_CAL_CLK, reg_val,
 					reg_val & MTK_PHY_DA_CAL_CLK, 500,
-					ANALOG_INTERNAL_OPERATION_MAX_US, false);
+					ANALOG_INTERNAL_OPERATION_MAX_US,
+					false);
 	if (ret) {
 		phydev_err(phydev, "Calibration cycle timeout\n");
 		return ret;
@@ -351,7 +305,7 @@  static int cal_cycle(struct phy_device *phydev, int devad,
 	phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_RG_AD_CALIN,
 			   MTK_PHY_DA_CALIN_FLAG);
 	ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_RG_AD_CAL_COMP) >>
-			   MTK_PHY_AD_CAL_COMP_OUT_SHIFT;
+	      MTK_PHY_AD_CAL_COMP_OUT_SHIFT;
 	phydev_dbg(phydev, "cal_val: 0x%x, ret: %d\n", cal_val, ret);
 
 	return ret;
@@ -440,38 +394,46 @@  static int tx_amp_fill_result(struct phy_device *phydev, u16 *buf)
 	}
 
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TXVLD_DA_RG,
-		       MTK_PHY_DA_TX_I2MPB_A_GBE_MASK, (buf[0] + bias[0]) << 10);
+		       MTK_PHY_DA_TX_I2MPB_A_GBE_MASK,
+		       (buf[0] + bias[0]) << 10);
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TXVLD_DA_RG,
 		       MTK_PHY_DA_TX_I2MPB_A_TBT_MASK, buf[0] + bias[1]);
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_A2,
-		       MTK_PHY_DA_TX_I2MPB_A_HBT_MASK, (buf[0] + bias[2]) << 10);
+		       MTK_PHY_DA_TX_I2MPB_A_HBT_MASK,
+		       (buf[0] + bias[2]) << 10);
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_A2,
 		       MTK_PHY_DA_TX_I2MPB_A_TST_MASK, buf[0] + bias[3]);
 
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_B1,
-		       MTK_PHY_DA_TX_I2MPB_B_GBE_MASK, (buf[1] + bias[4]) << 8);
+		       MTK_PHY_DA_TX_I2MPB_B_GBE_MASK,
+		       (buf[1] + bias[4]) << 8);
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_B1,
 		       MTK_PHY_DA_TX_I2MPB_B_TBT_MASK, buf[1] + bias[5]);
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_B2,
-		       MTK_PHY_DA_TX_I2MPB_B_HBT_MASK, (buf[1] + bias[6]) << 8);
+		       MTK_PHY_DA_TX_I2MPB_B_HBT_MASK,
+		       (buf[1] + bias[6]) << 8);
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_B2,
 		       MTK_PHY_DA_TX_I2MPB_B_TST_MASK, buf[1] + bias[7]);
 
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_C1,
-		       MTK_PHY_DA_TX_I2MPB_C_GBE_MASK, (buf[2] + bias[8]) << 8);
+		       MTK_PHY_DA_TX_I2MPB_C_GBE_MASK,
+		       (buf[2] + bias[8]) << 8);
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_C1,
 		       MTK_PHY_DA_TX_I2MPB_C_TBT_MASK, buf[2] + bias[9]);
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_C2,
-		       MTK_PHY_DA_TX_I2MPB_C_HBT_MASK, (buf[2] + bias[10]) << 8);
+		       MTK_PHY_DA_TX_I2MPB_C_HBT_MASK,
+		       (buf[2] + bias[10]) << 8);
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_C2,
 		       MTK_PHY_DA_TX_I2MPB_C_TST_MASK, buf[2] + bias[11]);
 
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_D1,
-		       MTK_PHY_DA_TX_I2MPB_D_GBE_MASK, (buf[3] + bias[12]) << 8);
+		       MTK_PHY_DA_TX_I2MPB_D_GBE_MASK,
+		       (buf[3] + bias[12]) << 8);
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_D1,
 		       MTK_PHY_DA_TX_I2MPB_D_TBT_MASK, buf[3] + bias[13]);
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_D2,
-		       MTK_PHY_DA_TX_I2MPB_D_HBT_MASK, (buf[3] + bias[14]) << 8);
+		       MTK_PHY_DA_TX_I2MPB_D_HBT_MASK,
+		       (buf[3] + bias[14]) << 8);
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_TX_I2MPB_TEST_MODE_D2,
 		       MTK_PHY_DA_TX_I2MPB_D_TST_MASK, buf[3] + bias[15]);
 
@@ -662,7 +624,8 @@  static int tx_vcm_cal_sw(struct phy_device *phydev, u8 rg_txreserve_x)
 		goto restore;
 
 	/* We calibrate TX-VCM in different logic. Check upper index and then
-	 * lower index. If this calibration is valid, apply lower index's result.
+	 * lower index. If this calibration is valid, apply lower index's
+	 * result.
 	 */
 	ret = upper_ret - lower_ret;
 	if (ret == 1) {
@@ -691,7 +654,8 @@  static int tx_vcm_cal_sw(struct phy_device *phydev, u8 rg_txreserve_x)
 	} else if (upper_idx == TXRESERVE_MAX && upper_ret == 0 &&
 		   lower_ret == 0) {
 		ret = 0;
-		phydev_warn(phydev, "TX-VCM SW cal result at high margin 0x%x\n",
+		phydev_warn(phydev,
+			    "TX-VCM SW cal result at high margin 0x%x\n",
 			    upper_idx);
 	} else {
 		ret = -EINVAL;
@@ -795,7 +759,8 @@  static void mt7981_phy_finetune(struct phy_device *phydev)
 
 	/* TR_OPEN_LOOP_EN = 1, lpf_x_average = 9 */
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_RG_DEV1E_REG234,
-		       MTK_PHY_TR_OPEN_LOOP_EN_MASK | MTK_PHY_LPF_X_AVERAGE_MASK,
+		       MTK_PHY_TR_OPEN_LOOP_EN_MASK |
+		       MTK_PHY_LPF_X_AVERAGE_MASK,
 		       BIT(0) | FIELD_PREP(MTK_PHY_LPF_X_AVERAGE_MASK, 0x9));
 
 	/* rg_tr_lpf_cnt_val = 512 */
@@ -864,7 +829,8 @@  static void mt7988_phy_finetune(struct phy_device *phydev)
 
 	/* TR_OPEN_LOOP_EN = 1, lpf_x_average = 10 */
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_RG_DEV1E_REG234,
-		       MTK_PHY_TR_OPEN_LOOP_EN_MASK | MTK_PHY_LPF_X_AVERAGE_MASK,
+		       MTK_PHY_TR_OPEN_LOOP_EN_MASK |
+		       MTK_PHY_LPF_X_AVERAGE_MASK,
 		       BIT(0) | FIELD_PREP(MTK_PHY_LPF_X_AVERAGE_MASK, 0xa));
 
 	/* rg_tr_lpf_cnt_val = 1023 */
@@ -976,7 +942,8 @@  static void mt798x_phy_eee(struct phy_device *phydev)
 	phy_restore_page(phydev, MTK_PHY_PAGE_STANDARD, 0);
 
 	phy_select_page(phydev, MTK_PHY_PAGE_EXTENDED_3);
-	__phy_modify(phydev, MTK_PHY_LPI_REG_14, MTK_PHY_LPI_WAKE_TIMER_1000_MASK,
+	__phy_modify(phydev, MTK_PHY_LPI_REG_14,
+		     MTK_PHY_LPI_WAKE_TIMER_1000_MASK,
 		     FIELD_PREP(MTK_PHY_LPI_WAKE_TIMER_1000_MASK, 0x19c));
 
 	__phy_modify(phydev, MTK_PHY_LPI_REG_1c, MTK_PHY_SMI_DET_ON_THRESH_MASK,
@@ -986,7 +953,8 @@  static void mt798x_phy_eee(struct phy_device *phydev)
 	phy_modify_mmd(phydev, MDIO_MMD_VEND1,
 		       MTK_PHY_RG_LPI_PCS_DSP_CTRL_REG122,
 		       MTK_PHY_LPI_NORM_MSE_HI_THRESH1000_MASK,
-		       FIELD_PREP(MTK_PHY_LPI_NORM_MSE_HI_THRESH1000_MASK, 0xff));
+		       FIELD_PREP(MTK_PHY_LPI_NORM_MSE_HI_THRESH1000_MASK,
+				  0xff));
 }
 
 static int cal_sw(struct phy_device *phydev, enum CAL_ITEM cal_item,
@@ -1130,57 +1098,13 @@  static int mt798x_phy_config_init(struct phy_device *phydev)
 	return mt798x_phy_calibration(phydev);
 }
 
-static int mt798x_phy_hw_led_on_set(struct phy_device *phydev, u8 index,
-				    bool on)
-{
-	unsigned int bit_on = MTK_PHY_LED_STATE_FORCE_ON + (index ? 16 : 0);
-	struct mtk_socphy_priv *priv = phydev->priv;
-	bool changed;
-
-	if (on)
-		changed = !test_and_set_bit(bit_on, &priv->led_state);
-	else
-		changed = !!test_and_clear_bit(bit_on, &priv->led_state);
-
-	changed |= !!test_and_clear_bit(MTK_PHY_LED_STATE_NETDEV +
-					(index ? 16 : 0), &priv->led_state);
-	if (changed)
-		return phy_modify_mmd(phydev, MDIO_MMD_VEND2, index ?
-				      MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL,
-				      MTK_PHY_LED_ON_MASK,
-				      on ? MTK_PHY_LED_ON_FORCE_ON : 0);
-	else
-		return 0;
-}
-
-static int mt798x_phy_hw_led_blink_set(struct phy_device *phydev, u8 index,
-				       bool blinking)
-{
-	unsigned int bit_blink = MTK_PHY_LED_STATE_FORCE_BLINK + (index ? 16 : 0);
-	struct mtk_socphy_priv *priv = phydev->priv;
-	bool changed;
-
-	if (blinking)
-		changed = !test_and_set_bit(bit_blink, &priv->led_state);
-	else
-		changed = !!test_and_clear_bit(bit_blink, &priv->led_state);
-
-	changed |= !!test_bit(MTK_PHY_LED_STATE_NETDEV +
-			      (index ? 16 : 0), &priv->led_state);
-	if (changed)
-		return phy_write_mmd(phydev, MDIO_MMD_VEND2, index ?
-				     MTK_PHY_LED1_BLINK_CTRL : MTK_PHY_LED0_BLINK_CTRL,
-				     blinking ? MTK_PHY_LED_BLINK_FORCE_BLINK : 0);
-	else
-		return 0;
-}
-
 static int mt798x_phy_led_blink_set(struct phy_device *phydev, u8 index,
 				    unsigned long *delay_on,
 				    unsigned long *delay_off)
 {
 	bool blinking = false;
 	int err = 0;
+	struct mtk_socphy_priv *priv = phydev->priv;
 
 	if (index > 1)
 		return -EINVAL;
@@ -1191,179 +1115,66 @@  static int mt798x_phy_led_blink_set(struct phy_device *phydev, u8 index,
 		*delay_off = 50;
 	}
 
-	err = mt798x_phy_hw_led_blink_set(phydev, index, blinking);
+	err = mtk_phy_hw_led_blink_set(phydev, index, &priv->led_state,
+				       blinking);
 	if (err)
 		return err;
 
-	return mt798x_phy_hw_led_on_set(phydev, index, false);
+	return mtk_phy_hw_led_on_set(phydev, index, &priv->led_state,
+				     MTK_GPHY_LED_ON_MASK, false);
 }
 
 static int mt798x_phy_led_brightness_set(struct phy_device *phydev,
 					 u8 index, enum led_brightness value)
 {
 	int err;
+	struct mtk_socphy_priv *priv = phydev->priv;
 
-	err = mt798x_phy_hw_led_blink_set(phydev, index, false);
+	err = mtk_phy_hw_led_blink_set(phydev, index, &priv->led_state, false);
 	if (err)
 		return err;
 
-	return mt798x_phy_hw_led_on_set(phydev, index, (value != LED_OFF));
+	return mtk_phy_hw_led_on_set(phydev, index, &priv->led_state,
+				     MTK_GPHY_LED_ON_MASK, (value != LED_OFF));
 }
 
-static const unsigned long supported_triggers = (BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
-						 BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
-						 BIT(TRIGGER_NETDEV_LINK)        |
-						 BIT(TRIGGER_NETDEV_LINK_10)     |
-						 BIT(TRIGGER_NETDEV_LINK_100)    |
-						 BIT(TRIGGER_NETDEV_LINK_1000)   |
-						 BIT(TRIGGER_NETDEV_RX)          |
-						 BIT(TRIGGER_NETDEV_TX));
+static const unsigned long supported_triggers =
+	(BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
+	 BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
+	 BIT(TRIGGER_NETDEV_LINK)        |
+	 BIT(TRIGGER_NETDEV_LINK_10)     |
+	 BIT(TRIGGER_NETDEV_LINK_100)    |
+	 BIT(TRIGGER_NETDEV_LINK_1000)   |
+	 BIT(TRIGGER_NETDEV_RX)          |
+	 BIT(TRIGGER_NETDEV_TX));
 
 static int mt798x_phy_led_hw_is_supported(struct phy_device *phydev, u8 index,
 					  unsigned long rules)
 {
-	if (index > 1)
-		return -EINVAL;
-
-	/* All combinations of the supported triggers are allowed */
-	if (rules & ~supported_triggers)
-		return -EOPNOTSUPP;
-
-	return 0;
-};
+	return mtk_phy_led_hw_is_supported(phydev, index, rules,
+					   supported_triggers);
+}
 
 static int mt798x_phy_led_hw_control_get(struct phy_device *phydev, u8 index,
 					 unsigned long *rules)
 {
-	unsigned int bit_blink = MTK_PHY_LED_STATE_FORCE_BLINK + (index ? 16 : 0);
-	unsigned int bit_netdev = MTK_PHY_LED_STATE_NETDEV + (index ? 16 : 0);
-	unsigned int bit_on = MTK_PHY_LED_STATE_FORCE_ON + (index ? 16 : 0);
 	struct mtk_socphy_priv *priv = phydev->priv;
-	int on, blink;
-
-	if (index > 1)
-		return -EINVAL;
-
-	on = phy_read_mmd(phydev, MDIO_MMD_VEND2,
-			  index ? MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL);
 
-	if (on < 0)
-		return -EIO;
-
-	blink = phy_read_mmd(phydev, MDIO_MMD_VEND2,
-			     index ? MTK_PHY_LED1_BLINK_CTRL :
-				     MTK_PHY_LED0_BLINK_CTRL);
-	if (blink < 0)
-		return -EIO;
-
-	if ((on & (MTK_PHY_LED_ON_LINK | MTK_PHY_LED_ON_FDX | MTK_PHY_LED_ON_HDX |
-		   MTK_PHY_LED_ON_LINKDOWN)) ||
-	    (blink & (MTK_PHY_LED_BLINK_RX | MTK_PHY_LED_BLINK_TX)))
-		set_bit(bit_netdev, &priv->led_state);
-	else
-		clear_bit(bit_netdev, &priv->led_state);
-
-	if (on & MTK_PHY_LED_ON_FORCE_ON)
-		set_bit(bit_on, &priv->led_state);
-	else
-		clear_bit(bit_on, &priv->led_state);
-
-	if (blink & MTK_PHY_LED_BLINK_FORCE_BLINK)
-		set_bit(bit_blink, &priv->led_state);
-	else
-		clear_bit(bit_blink, &priv->led_state);
-
-	if (!rules)
-		return 0;
-
-	if (on & MTK_PHY_LED_ON_LINK)
-		*rules |= BIT(TRIGGER_NETDEV_LINK);
-
-	if (on & MTK_PHY_LED_ON_LINK10)
-		*rules |= BIT(TRIGGER_NETDEV_LINK_10);
-
-	if (on & MTK_PHY_LED_ON_LINK100)
-		*rules |= BIT(TRIGGER_NETDEV_LINK_100);
-
-	if (on & MTK_PHY_LED_ON_LINK1000)
-		*rules |= BIT(TRIGGER_NETDEV_LINK_1000);
-
-	if (on & MTK_PHY_LED_ON_FDX)
-		*rules |= BIT(TRIGGER_NETDEV_FULL_DUPLEX);
-
-	if (on & MTK_PHY_LED_ON_HDX)
-		*rules |= BIT(TRIGGER_NETDEV_HALF_DUPLEX);
-
-	if (blink & MTK_PHY_LED_BLINK_RX)
-		*rules |= BIT(TRIGGER_NETDEV_RX);
-
-	if (blink & MTK_PHY_LED_BLINK_TX)
-		*rules |= BIT(TRIGGER_NETDEV_TX);
-
-	return 0;
+	return mtk_phy_led_hw_ctrl_get(phydev, index, rules, &priv->led_state,
+				       MTK_GPHY_LED_ON_SET,
+				       MTK_GPHY_LED_RX_BLINK_SET,
+				       MTK_GPHY_LED_TX_BLINK_SET);
 };
 
 static int mt798x_phy_led_hw_control_set(struct phy_device *phydev, u8 index,
 					 unsigned long rules)
 {
-	unsigned int bit_netdev = MTK_PHY_LED_STATE_NETDEV + (index ? 16 : 0);
 	struct mtk_socphy_priv *priv = phydev->priv;
-	u16 on = 0, blink = 0;
-	int ret;
-
-	if (index > 1)
-		return -EINVAL;
-
-	if (rules & BIT(TRIGGER_NETDEV_FULL_DUPLEX))
-		on |= MTK_PHY_LED_ON_FDX;
-
-	if (rules & BIT(TRIGGER_NETDEV_HALF_DUPLEX))
-		on |= MTK_PHY_LED_ON_HDX;
-
-	if (rules & (BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK)))
-		on |= MTK_PHY_LED_ON_LINK10;
-
-	if (rules & (BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK)))
-		on |= MTK_PHY_LED_ON_LINK100;
-
-	if (rules & (BIT(TRIGGER_NETDEV_LINK_1000) | BIT(TRIGGER_NETDEV_LINK)))
-		on |= MTK_PHY_LED_ON_LINK1000;
 
-	if (rules & BIT(TRIGGER_NETDEV_RX)) {
-		blink |= (on & MTK_PHY_LED_ON_LINK) ?
-			  (((on & MTK_PHY_LED_ON_LINK10) ? MTK_PHY_LED_BLINK_10RX : 0) |
-			   ((on & MTK_PHY_LED_ON_LINK100) ? MTK_PHY_LED_BLINK_100RX : 0) |
-			   ((on & MTK_PHY_LED_ON_LINK1000) ? MTK_PHY_LED_BLINK_1000RX : 0)) :
-			  MTK_PHY_LED_BLINK_RX;
-	}
-
-	if (rules & BIT(TRIGGER_NETDEV_TX)) {
-		blink |= (on & MTK_PHY_LED_ON_LINK) ?
-			  (((on & MTK_PHY_LED_ON_LINK10) ? MTK_PHY_LED_BLINK_10TX : 0) |
-			   ((on & MTK_PHY_LED_ON_LINK100) ? MTK_PHY_LED_BLINK_100TX : 0) |
-			   ((on & MTK_PHY_LED_ON_LINK1000) ? MTK_PHY_LED_BLINK_1000TX : 0)) :
-			  MTK_PHY_LED_BLINK_TX;
-	}
-
-	if (blink || on)
-		set_bit(bit_netdev, &priv->led_state);
-	else
-		clear_bit(bit_netdev, &priv->led_state);
-
-	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, index ?
-				MTK_PHY_LED1_ON_CTRL :
-				MTK_PHY_LED0_ON_CTRL,
-			     MTK_PHY_LED_ON_FDX     |
-			     MTK_PHY_LED_ON_HDX     |
-			     MTK_PHY_LED_ON_LINK,
-			     on);
-
-	if (ret)
-		return ret;
-
-	return phy_write_mmd(phydev, MDIO_MMD_VEND2, index ?
-				MTK_PHY_LED1_BLINK_CTRL :
-				MTK_PHY_LED0_BLINK_CTRL, blink);
+	return mtk_phy_led_hw_ctrl_set(phydev, index, rules, &priv->led_state,
+				       MTK_GPHY_LED_ON_SET,
+				       MTK_GPHY_LED_RX_BLINK_SET,
+				       MTK_GPHY_LED_TX_BLINK_SET);
 };
 
 static bool mt7988_phy_led_get_polarity(struct phy_device *phydev, int led_num)
@@ -1390,15 +1201,16 @@  static int mt7988_phy_fix_leds_polarities(struct phy_device *phydev)
 	/* Setup LED polarity according to bootstrap use of LED pins */
 	for (index = 0; index < 2; ++index)
 		phy_modify_mmd(phydev, MDIO_MMD_VEND2, index ?
-				MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL,
+			       MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL,
 			       MTK_PHY_LED_ON_POLARITY,
 			       mt7988_phy_led_get_polarity(phydev, index) ?
-				MTK_PHY_LED_ON_POLARITY : 0);
+			       MTK_PHY_LED_ON_POLARITY : 0);
 
 	/* Only now setup pinctrl to avoid bogus blinking */
 	pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "gbe-led");
 	if (IS_ERR(pinctrl))
-		dev_err(&phydev->mdio.bus->dev, "Failed to setup PHY LED pinctrl\n");
+		dev_err(&phydev->mdio.bus->dev,
+			"Failed to setup PHY LED pinctrl\n");
 
 	return 0;
 }
@@ -1437,14 +1249,6 @@  static int mt7988_phy_probe_shared(struct phy_device *phydev)
 	return 0;
 }
 
-static void mt798x_phy_leds_state_init(struct phy_device *phydev)
-{
-	int i;
-
-	for (i = 0; i < 2; ++i)
-		mt798x_phy_led_hw_control_get(phydev, i, NULL);
-}
-
 static int mt7988_phy_probe(struct phy_device *phydev)
 {
 	struct mtk_socphy_shared *shared;
@@ -1470,7 +1274,7 @@  static int mt7988_phy_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
-	mt798x_phy_leds_state_init(phydev);
+	mtk_phy_leds_state_init(phydev);
 
 	err = mt7988_phy_fix_leds_polarities(phydev);
 	if (err)
@@ -1497,7 +1301,7 @@  static int mt7981_phy_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
-	mt798x_phy_leds_state_init(phydev);
+	mtk_phy_leds_state_init(phydev);
 
 	return mt798x_phy_calibration(phydev);
 }
@@ -1512,8 +1316,8 @@  static struct phy_driver mtk_socphy_driver[] = {
 		.probe		= mt7981_phy_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
-		.read_page	= mtk_socphy_read_page,
-		.write_page	= mtk_socphy_write_page,
+		.read_page	= mtk_phy_read_page,
+		.write_page	= mtk_phy_write_page,
 		.led_blink_set	= mt798x_phy_led_blink_set,
 		.led_brightness_set = mt798x_phy_led_brightness_set,
 		.led_hw_is_supported = mt798x_phy_led_hw_is_supported,
@@ -1529,8 +1333,8 @@  static struct phy_driver mtk_socphy_driver[] = {
 		.probe		= mt7988_phy_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
-		.read_page	= mtk_socphy_read_page,
-		.write_page	= mtk_socphy_write_page,
+		.read_page	= mtk_phy_read_page,
+		.write_page	= mtk_phy_write_page,
 		.led_blink_set	= mt798x_phy_led_blink_set,
 		.led_brightness_set = mt798x_phy_led_brightness_set,
 		.led_hw_is_supported = mt798x_phy_led_hw_is_supported,
diff --git a/drivers/net/phy/mediatek/mtk-ge.c b/drivers/net/phy/mediatek/mtk-ge.c
index 54ea64a..4022f98 100644
--- a/drivers/net/phy/mediatek/mtk-ge.c
+++ b/drivers/net/phy/mediatek/mtk-ge.c
@@ -3,6 +3,11 @@ 
 #include <linux/module.h>
 #include <linux/phy.h>
 
+#include "mtk.h"
+
+#define MTK_GPHY_ID_MT7530		0x03a29412
+#define MTK_GPHY_ID_MT7531		0x03a29441
+
 #define MTK_EXT_PAGE_ACCESS		0x1f
 #define MTK_PHY_PAGE_STANDARD		0x0000
 #define MTK_PHY_PAGE_EXTENDED		0x0001
@@ -11,15 +16,9 @@ 
 #define MTK_PHY_PAGE_EXTENDED_2A30	0x2a30
 #define MTK_PHY_PAGE_EXTENDED_52B5	0x52b5
 
-static int mtk_gephy_read_page(struct phy_device *phydev)
-{
-	return __phy_read(phydev, MTK_EXT_PAGE_ACCESS);
-}
-
-static int mtk_gephy_write_page(struct phy_device *phydev, int page)
-{
-	return __phy_write(phydev, MTK_EXT_PAGE_ACCESS, page);
-}
+struct mtk_gephy_priv {
+	unsigned long		led_state;
+};
 
 static void mtk_gephy_config_init(struct phy_device *phydev)
 {
@@ -65,9 +64,104 @@  static int mt7531_phy_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int mt7531_phy_probe(struct phy_device *phydev)
+{
+	struct mtk_gephy_priv *priv;
+
+	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(struct mtk_gephy_priv),
+			    GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phydev->priv = priv;
+
+	mtk_phy_leds_state_init(phydev);
+
+	return 0;
+}
+
+static int mt753x_phy_led_blink_set(struct phy_device *phydev, u8 index,
+				    unsigned long *delay_on,
+				    unsigned long *delay_off)
+{
+	bool blinking = false;
+	int err = 0;
+	struct mtk_gephy_priv *priv = phydev->priv;
+
+	if (index > 1)
+		return -EINVAL;
+
+	if (delay_on && delay_off && (*delay_on > 0) && (*delay_off > 0)) {
+		blinking = true;
+		*delay_on = 50;
+		*delay_off = 50;
+	}
+
+	err = mtk_phy_hw_led_blink_set(phydev, index, &priv->led_state,
+				       blinking);
+	if (err)
+		return err;
+
+	return mtk_phy_hw_led_on_set(phydev, index, &priv->led_state,
+				     MTK_GPHY_LED_ON_MASK, false);
+}
+
+static int mt753x_phy_led_brightness_set(struct phy_device *phydev,
+					 u8 index, enum led_brightness value)
+{
+	int err;
+	struct mtk_gephy_priv *priv = phydev->priv;
+
+	err = mtk_phy_hw_led_blink_set(phydev, index, &priv->led_state, false);
+	if (err)
+		return err;
+
+	return mtk_phy_hw_led_on_set(phydev, index, &priv->led_state,
+				     MTK_GPHY_LED_ON_MASK, (value != LED_OFF));
+}
+
+static const unsigned long supported_triggers =
+	(BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
+	 BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
+	 BIT(TRIGGER_NETDEV_LINK)        |
+	 BIT(TRIGGER_NETDEV_LINK_10)     |
+	 BIT(TRIGGER_NETDEV_LINK_100)    |
+	 BIT(TRIGGER_NETDEV_LINK_1000)   |
+	 BIT(TRIGGER_NETDEV_RX)          |
+	 BIT(TRIGGER_NETDEV_TX));
+
+static int mt753x_phy_led_hw_is_supported(struct phy_device *phydev, u8 index,
+					  unsigned long rules)
+{
+	return mtk_phy_led_hw_is_supported(phydev, index, rules,
+					   supported_triggers);
+}
+
+static int mt753x_phy_led_hw_control_get(struct phy_device *phydev, u8 index,
+					 unsigned long *rules)
+{
+	struct mtk_gephy_priv *priv = phydev->priv;
+
+	return mtk_phy_led_hw_ctrl_get(phydev, index, rules, &priv->led_state,
+				       MTK_GPHY_LED_ON_SET,
+				       MTK_GPHY_LED_RX_BLINK_SET,
+				       MTK_GPHY_LED_TX_BLINK_SET);
+};
+
+static int mt753x_phy_led_hw_control_set(struct phy_device *phydev, u8 index,
+					 unsigned long rules)
+{
+	struct mtk_gephy_priv *priv = phydev->priv;
+
+	return mtk_phy_led_hw_ctrl_set(phydev, index, rules, &priv->led_state,
+				       MTK_GPHY_LED_ON_SET,
+				       MTK_GPHY_LED_RX_BLINK_SET,
+				       MTK_GPHY_LED_TX_BLINK_SET);
+};
+
 static struct phy_driver mtk_gephy_driver[] = {
 	{
-		PHY_ID_MATCH_EXACT(0x03a29412),
+		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530),
 		.name		= "MediaTek MT7530 PHY",
 		.config_init	= mt7530_phy_config_init,
 		/* Interrupts are handled by the switch, not the PHY
@@ -77,12 +171,14 @@  static struct phy_driver mtk_gephy_driver[] = {
 		.handle_interrupt = genphy_handle_interrupt_no_ack,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
-		.read_page	= mtk_gephy_read_page,
-		.write_page	= mtk_gephy_write_page,
+		.read_page	= mtk_phy_read_page,
+		.write_page	= mtk_phy_write_page,
+		.led_hw_is_supported = mt753x_phy_led_hw_is_supported,
 	},
 	{
-		PHY_ID_MATCH_EXACT(0x03a29441),
+		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7531),
 		.name		= "MediaTek MT7531 PHY",
+		.probe		= mt7531_phy_probe,
 		.config_init	= mt7531_phy_config_init,
 		/* Interrupts are handled by the switch, not the PHY
 		 * itself.
@@ -91,16 +187,21 @@  static struct phy_driver mtk_gephy_driver[] = {
 		.handle_interrupt = genphy_handle_interrupt_no_ack,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
-		.read_page	= mtk_gephy_read_page,
-		.write_page	= mtk_gephy_write_page,
+		.read_page	= mtk_phy_read_page,
+		.write_page	= mtk_phy_write_page,
+		.led_blink_set	= mt753x_phy_led_blink_set,
+		.led_brightness_set = mt753x_phy_led_brightness_set,
+		.led_hw_is_supported = mt753x_phy_led_hw_is_supported,
+		.led_hw_control_set = mt753x_phy_led_hw_control_set,
+		.led_hw_control_get = mt753x_phy_led_hw_control_get,
 	},
 };
 
 module_phy_driver(mtk_gephy_driver);
 
 static struct mdio_device_id __maybe_unused mtk_gephy_tbl[] = {
-	{ PHY_ID_MATCH_EXACT(0x03a29441) },
-	{ PHY_ID_MATCH_EXACT(0x03a29412) },
+	{ PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530) },
+	{ PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7531) },
 	{ }
 };
 
diff --git a/drivers/net/phy/mediatek/mtk-phy-lib.c b/drivers/net/phy/mediatek/mtk-phy-lib.c
new file mode 100644
index 0000000..d892f94
--- /dev/null
+++ b/drivers/net/phy/mediatek/mtk-phy-lib.c
@@ -0,0 +1,247 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/phy.h>
+#include <linux/module.h>
+
+#include <linux/netdevice.h>
+
+#include "mtk.h"
+
+int mtk_phy_read_page(struct phy_device *phydev)
+{
+	return __phy_read(phydev, MTK_EXT_PAGE_ACCESS);
+}
+EXPORT_SYMBOL_GPL(mtk_phy_read_page);
+
+int mtk_phy_write_page(struct phy_device *phydev, int page)
+{
+	return __phy_write(phydev, MTK_EXT_PAGE_ACCESS, page);
+}
+EXPORT_SYMBOL_GPL(mtk_phy_write_page);
+
+int mtk_phy_led_hw_is_supported(struct phy_device *phydev, u8 index,
+				unsigned long rules,
+				unsigned long supported_triggers)
+{
+	if (index > 1)
+		return -EINVAL;
+
+	/* All combinations of the supported triggers are allowed */
+	if (rules & ~supported_triggers)
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mtk_phy_led_hw_is_supported);
+
+int mtk_phy_led_hw_ctrl_get(struct phy_device *phydev, u8 index,
+			    unsigned long *rules, unsigned long *led_state,
+			    u16 on_set, u16 rx_blink_set, u16 tx_blink_set)
+{
+	unsigned int bit_blink = MTK_PHY_LED_STATE_FORCE_BLINK +
+				 (index ? 16 : 0);
+	unsigned int bit_netdev = MTK_PHY_LED_STATE_NETDEV + (index ? 16 : 0);
+	unsigned int bit_on = MTK_PHY_LED_STATE_FORCE_ON + (index ? 16 : 0);
+	int on, blink;
+
+	if (index > 1)
+		return -EINVAL;
+
+	on = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+			  index ? MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL);
+
+	if (on < 0)
+		return -EIO;
+
+	blink = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+			     index ? MTK_PHY_LED1_BLINK_CTRL :
+				     MTK_PHY_LED0_BLINK_CTRL);
+	if (blink < 0)
+		return -EIO;
+
+	if ((on & (on_set | MTK_PHY_LED_ON_FDX |
+		   MTK_PHY_LED_ON_HDX | MTK_PHY_LED_ON_LINKDOWN)) ||
+	    (blink & (rx_blink_set | tx_blink_set)))
+		set_bit(bit_netdev, led_state);
+	else
+		clear_bit(bit_netdev, led_state);
+
+	if (on & MTK_PHY_LED_ON_FORCE_ON)
+		set_bit(bit_on, led_state);
+	else
+		clear_bit(bit_on, led_state);
+
+	if (blink & MTK_PHY_LED_BLINK_FORCE_BLINK)
+		set_bit(bit_blink, led_state);
+	else
+		clear_bit(bit_blink, led_state);
+
+	if (!rules)
+		return 0;
+
+	if (on & on_set)
+		*rules |= BIT(TRIGGER_NETDEV_LINK);
+
+	if (on & MTK_PHY_LED_ON_LINK10)
+		*rules |= BIT(TRIGGER_NETDEV_LINK_10);
+
+	if (on & MTK_PHY_LED_ON_LINK100)
+		*rules |= BIT(TRIGGER_NETDEV_LINK_100);
+
+	if (on & MTK_PHY_LED_ON_LINK1000)
+		*rules |= BIT(TRIGGER_NETDEV_LINK_1000);
+
+	if (on & MTK_PHY_LED_ON_LINK2500)
+		*rules |= BIT(TRIGGER_NETDEV_LINK_2500);
+
+	if (on & MTK_PHY_LED_ON_FDX)
+		*rules |= BIT(TRIGGER_NETDEV_FULL_DUPLEX);
+
+	if (on & MTK_PHY_LED_ON_HDX)
+		*rules |= BIT(TRIGGER_NETDEV_HALF_DUPLEX);
+
+	if (blink & rx_blink_set)
+		*rules |= BIT(TRIGGER_NETDEV_RX);
+
+	if (blink & tx_blink_set)
+		*rules |= BIT(TRIGGER_NETDEV_TX);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mtk_phy_led_hw_ctrl_get);
+
+int mtk_phy_led_hw_ctrl_set(struct phy_device *phydev, u8 index,
+			    unsigned long rules, unsigned long *led_state,
+			    u16 on_set, u16 rx_blink_set, u16 tx_blink_set)
+{
+	unsigned int bit_netdev = MTK_PHY_LED_STATE_NETDEV + (index ? 16 : 0);
+	u16 on = 0, blink = 0;
+	int ret;
+
+	if (index > 1)
+		return -EINVAL;
+
+	if (rules & BIT(TRIGGER_NETDEV_FULL_DUPLEX))
+		on |= MTK_PHY_LED_ON_FDX;
+
+	if (rules & BIT(TRIGGER_NETDEV_HALF_DUPLEX))
+		on |= MTK_PHY_LED_ON_HDX;
+
+	if (rules & (BIT(TRIGGER_NETDEV_LINK_10) | BIT(TRIGGER_NETDEV_LINK)))
+		on |= MTK_PHY_LED_ON_LINK10;
+
+	if (rules & (BIT(TRIGGER_NETDEV_LINK_100) | BIT(TRIGGER_NETDEV_LINK)))
+		on |= MTK_PHY_LED_ON_LINK100;
+
+	if (rules & (BIT(TRIGGER_NETDEV_LINK_1000) | BIT(TRIGGER_NETDEV_LINK)))
+		on |= MTK_PHY_LED_ON_LINK1000;
+
+	if (rules & (BIT(TRIGGER_NETDEV_LINK_2500) | BIT(TRIGGER_NETDEV_LINK)))
+		on |= MTK_PHY_LED_ON_LINK2500;
+
+	if (rules & BIT(TRIGGER_NETDEV_RX)) {
+		blink |= (on & on_set) ?
+			  (((on & MTK_PHY_LED_ON_LINK10) ?
+			    MTK_PHY_LED_BLINK_10RX : 0) |
+			   ((on & MTK_PHY_LED_ON_LINK100) ?
+			    MTK_PHY_LED_BLINK_100RX : 0) |
+			   ((on & MTK_PHY_LED_ON_LINK1000) ?
+			    MTK_PHY_LED_BLINK_1000RX : 0) |
+			   ((on & MTK_PHY_LED_ON_LINK2500) ?
+			    MTK_PHY_LED_BLINK_2500RX : 0)) :
+			  rx_blink_set;
+	}
+
+	if (rules & BIT(TRIGGER_NETDEV_TX)) {
+		blink |= (on & on_set) ?
+			  (((on & MTK_PHY_LED_ON_LINK10) ?
+			    MTK_PHY_LED_BLINK_10TX : 0) |
+			   ((on & MTK_PHY_LED_ON_LINK100) ?
+			    MTK_PHY_LED_BLINK_100TX : 0) |
+			   ((on & MTK_PHY_LED_ON_LINK1000) ?
+			    MTK_PHY_LED_BLINK_1000TX : 0) |
+			   ((on & MTK_PHY_LED_ON_LINK2500) ?
+			    MTK_PHY_LED_BLINK_2500TX : 0)) :
+			  tx_blink_set;
+	}
+
+	if (blink || on)
+		set_bit(bit_netdev, led_state);
+	else
+		clear_bit(bit_netdev, led_state);
+
+	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, index ?
+			     MTK_PHY_LED1_ON_CTRL : MTK_PHY_LED0_ON_CTRL,
+			     MTK_PHY_LED_ON_FDX | MTK_PHY_LED_ON_HDX | on_set,
+			     on);
+
+	if (ret)
+		return ret;
+
+	return phy_write_mmd(phydev, MDIO_MMD_VEND2, index ?
+			     MTK_PHY_LED1_BLINK_CTRL :
+			     MTK_PHY_LED0_BLINK_CTRL, blink);
+}
+EXPORT_SYMBOL_GPL(mtk_phy_led_hw_ctrl_set);
+
+int mtk_phy_hw_led_on_set(struct phy_device *phydev, u8 index,
+			  unsigned long *led_state, u16 led_on_mask, bool on)
+{
+	unsigned int bit_on = MTK_PHY_LED_STATE_FORCE_ON + (index ? 16 : 0);
+	bool changed;
+
+	if (on)
+		changed = !test_and_set_bit(bit_on, led_state);
+	else
+		changed = !!test_and_clear_bit(bit_on, led_state);
+
+	changed |= !!test_and_clear_bit(MTK_PHY_LED_STATE_NETDEV +
+					(index ? 16 : 0), led_state);
+	if (changed)
+		return phy_modify_mmd(phydev, MDIO_MMD_VEND2, index ?
+				      MTK_PHY_LED1_ON_CTRL :
+				      MTK_PHY_LED0_ON_CTRL,
+				      led_on_mask,
+				      on ? MTK_PHY_LED_ON_FORCE_ON : 0);
+	else
+		return 0;
+}
+EXPORT_SYMBOL_GPL(mtk_phy_hw_led_on_set);
+
+int mtk_phy_hw_led_blink_set(struct phy_device *phydev, u8 index,
+			     unsigned long *led_state, bool blinking)
+{
+	unsigned int bit_blink = MTK_PHY_LED_STATE_FORCE_BLINK +
+				 (index ? 16 : 0);
+	bool changed;
+
+	if (blinking)
+		changed = !test_and_set_bit(bit_blink, led_state);
+	else
+		changed = !!test_and_clear_bit(bit_blink, led_state);
+
+	changed |= !!test_bit(MTK_PHY_LED_STATE_NETDEV +
+			      (index ? 16 : 0), led_state);
+	if (changed)
+		return phy_write_mmd(phydev, MDIO_MMD_VEND2, index ?
+				     MTK_PHY_LED1_BLINK_CTRL :
+				     MTK_PHY_LED0_BLINK_CTRL,
+				     blinking ?
+				     MTK_PHY_LED_BLINK_FORCE_BLINK : 0);
+	else
+		return 0;
+}
+EXPORT_SYMBOL_GPL(mtk_phy_hw_led_blink_set);
+
+void mtk_phy_leds_state_init(struct phy_device *phydev)
+{
+	int i;
+
+	for (i = 0; i < 2; ++i)
+		phydev->drv->led_hw_control_get(phydev, i, NULL);
+}
+EXPORT_SYMBOL_GPL(mtk_phy_leds_state_init);
+
+MODULE_DESCRIPTION("MediaTek Ethernet PHY driver common");
+MODULE_AUTHOR("Sky Huang <SkyLake.Huang@mediatek.com>");
+MODULE_AUTHOR("Daniel Golle <daniel@makrotopia.org>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/phy/mediatek/mtk.h b/drivers/net/phy/mediatek/mtk.h
new file mode 100644
index 0000000..9cc9577
--- /dev/null
+++ b/drivers/net/phy/mediatek/mtk.h
@@ -0,0 +1,83 @@ 
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Common definition for Mediatek Ethernet PHYs
+ * Author: SkyLake Huang <SkyLake.Huang@mediatek.com>
+ * Copyright (c) 2024 MediaTek Inc.
+ */
+
+#ifndef _MTK_EPHY_H_
+#define _MTK_EPHY_H_
+
+#define MTK_EXT_PAGE_ACCESS			0x1f
+
+/* Registers on MDIO_MMD_VEND2 */
+#define MTK_PHY_LED0_ON_CTRL			0x24
+#define MTK_PHY_LED1_ON_CTRL			0x26
+#define   MTK_GPHY_LED_ON_MASK			GENMASK(6, 0)
+#define   MTK_2P5GPHY_LED_ON_MASK		GENMASK(7, 0)
+#define   MTK_PHY_LED_ON_LINK1000		BIT(0)
+#define   MTK_PHY_LED_ON_LINK100		BIT(1)
+#define   MTK_PHY_LED_ON_LINK10			BIT(2)
+#define   MTK_PHY_LED_ON_LINKDOWN		BIT(3)
+#define   MTK_PHY_LED_ON_FDX			BIT(4) /* Full duplex */
+#define   MTK_PHY_LED_ON_HDX			BIT(5) /* Half duplex */
+#define   MTK_PHY_LED_ON_FORCE_ON		BIT(6)
+#define   MTK_PHY_LED_ON_LINK2500		BIT(7)
+#define   MTK_PHY_LED_ON_POLARITY		BIT(14)
+#define   MTK_PHY_LED_ON_ENABLE			BIT(15)
+
+#define MTK_PHY_LED0_BLINK_CTRL			0x25
+#define MTK_PHY_LED1_BLINK_CTRL			0x27
+#define   MTK_PHY_LED_BLINK_1000TX		BIT(0)
+#define   MTK_PHY_LED_BLINK_1000RX		BIT(1)
+#define   MTK_PHY_LED_BLINK_100TX		BIT(2)
+#define   MTK_PHY_LED_BLINK_100RX		BIT(3)
+#define   MTK_PHY_LED_BLINK_10TX		BIT(4)
+#define   MTK_PHY_LED_BLINK_10RX		BIT(5)
+#define   MTK_PHY_LED_BLINK_COLLISION		BIT(6)
+#define   MTK_PHY_LED_BLINK_RX_CRC_ERR		BIT(7)
+#define   MTK_PHY_LED_BLINK_RX_IDLE_ERR		BIT(8)
+#define   MTK_PHY_LED_BLINK_FORCE_BLINK		BIT(9)
+#define   MTK_PHY_LED_BLINK_2500TX		BIT(10)
+#define   MTK_PHY_LED_BLINK_2500RX		BIT(11)
+
+#define MTK_GPHY_LED_ON_SET			(MTK_PHY_LED_ON_LINK1000 | \
+						 MTK_PHY_LED_ON_LINK100 | \
+						 MTK_PHY_LED_ON_LINK10)
+#define MTK_GPHY_LED_RX_BLINK_SET		(MTK_PHY_LED_BLINK_1000RX | \
+						 MTK_PHY_LED_BLINK_100RX | \
+						 MTK_PHY_LED_BLINK_10RX)
+#define MTK_GPHY_LED_TX_BLINK_SET		(MTK_PHY_LED_BLINK_1000RX | \
+						 MTK_PHY_LED_BLINK_100RX | \
+						 MTK_PHY_LED_BLINK_10RX)
+
+#define MTK_2P5GPHY_LED_ON_SET			(MTK_PHY_LED_ON_LINK2500 | \
+						 MTK_GPHY_LED_ON_SET)
+#define MTK_2P5GPHY_LED_RX_BLINK_SET		(MTK_PHY_LED_BLINK_2500RX | \
+						 MTK_GPHY_LED_RX_BLINK_SET)
+#define MTK_2P5GPHY_LED_TX_BLINK_SET		(MTK_PHY_LED_BLINK_2500RX | \
+						 MTK_GPHY_LED_TX_BLINK_SET)
+
+#define MTK_PHY_LED_STATE_FORCE_ON	0
+#define MTK_PHY_LED_STATE_FORCE_BLINK	1
+#define MTK_PHY_LED_STATE_NETDEV	2
+
+int mtk_phy_read_page(struct phy_device *phydev);
+int mtk_phy_write_page(struct phy_device *phydev, int page);
+
+int mtk_phy_led_hw_is_supported(struct phy_device *phydev, u8 index,
+				unsigned long rules,
+				unsigned long supported_triggers);
+int mtk_phy_led_hw_ctrl_set(struct phy_device *phydev, u8 index,
+			    unsigned long rules, unsigned long *led_state,
+			    u16 on_set, u16 rx_blink_set, u16 tx_blink_set);
+int mtk_phy_led_hw_ctrl_get(struct phy_device *phydev, u8 index,
+			    unsigned long *rules, unsigned long *led_state,
+			    u16 on_set, u16 rx_blink_set, u16 tx_blink_set);
+int mtk_phy_hw_led_on_set(struct phy_device *phydev, u8 index,
+			  unsigned long *led_state, u16 led_on_mask, bool on);
+int mtk_phy_hw_led_blink_set(struct phy_device *phydev, u8 index,
+			     unsigned long *led_state, bool blinking);
+void mtk_phy_leds_state_init(struct phy_device *phydev);
+
+#endif /* _MTK_EPHY_H_ */