Message-ID: <d9514bc8c18e1c7a1f6e3962f806f5cf23903a82.camel@mediatek.com>
Date: Wed, 19 Jun 2024 11:21:15 +0000
From: SkyLake Huang (黃啟澤)
	<SkyLake.Huang@...iatek.com>
To: "linux@...linux.org.uk" <linux@...linux.org.uk>
CC: "andrew@...n.ch" <andrew@...n.ch>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-mediatek@...ts.infradead.org"
	<linux-mediatek@...ts.infradead.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"dqfext@...il.com" <dqfext@...il.com>,
	Steven Liu (劉人豪) <steven.liu@...iatek.com>,
	"matthias.bgg@...il.com" <matthias.bgg@...il.com>, "davem@...emloft.net"
	<davem@...emloft.net>, "hkallweit1@...il.com" <hkallweit1@...il.com>,
	"daniel@...rotopia.org" <daniel@...rotopia.org>,
	"angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH net-next v7 2/5] net: phy: mediatek: Move LED and
 read/write page helper functions into mtk phy lib

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

Powered by blists - more mailing lists