diff mbox series

[net-next] net: phy: at803x: better align function varibles to open parenthesis

Message ID 20231217235011.2070-1-ansuelsmth@gmail.com
State New
Headers
Series [net-next] net: phy: at803x: better align function varibles to open parenthesis |

Commit Message

Christian Marangi Dec. 17, 2023, 11:50 p.m. UTC 
  Better align function variables to open parenthesis as suggested by
checkpatch script for qca808x function to make code cleaner.

For cable_test_get_status function some additional rework was needed to
handle too long functions.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 65 ++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 29 deletions(-)

Comments

Andrew Lunn Dec. 18, 2023, 9:47 a.m. UTC | #1 
On Mon, Dec 18, 2023 at 12:50:11AM +0100, Christian Marangi wrote:
> Better align function variables to open parenthesis as suggested by
> checkpatch script for qca808x function to make code cleaner.
> 
> For cable_test_get_status function some additional rework was needed to
> handle too long functions.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Russell King (Oracle) Jan. 2, 2024, 9:53 a.m. UTC | #2 
On Mon, Dec 18, 2023 at 12:50:11AM +0100, Christian Marangi wrote:
> -	if (qca808x_cdt_fault_length_valid(pair_a))
> -		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A,
> -				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A));
> -	if (qca808x_cdt_fault_length_valid(pair_b))
> -		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B,
> -				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B));
> -	if (qca808x_cdt_fault_length_valid(pair_c))
> -		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C,
> -				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C));
> -	if (qca808x_cdt_fault_length_valid(pair_d))
> -		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D,
> -				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D));
> +	if (qca808x_cdt_fault_length_valid(pair_a)) {
> +		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A);
> +		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A, val);
> +	}
> +	if (qca808x_cdt_fault_length_valid(pair_b)) {
> +		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B);
> +		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B, val);
> +	}
> +	if (qca808x_cdt_fault_length_valid(pair_c)) {
> +		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C);
> +		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C, val);
> +	}
> +	if (qca808x_cdt_fault_length_valid(pair_d)) {
> +		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D);
> +		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D, val);
> +	}

Maybe a static function for these?

static void qca808x_get_cdt_length(struct phy_device *phydev, u8 pair)
{
	ethnl_cable_test_fault_length(phydev, pair,
				      qca808x_cdt_fault_length(phydev, pair));
}

or going via 'val'. Either way, repeating the same two lines multiple
times seems a bit suboptimal.

Whether or not you do this:

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!
Christian Marangi Jan. 2, 2024, 10 a.m. UTC | #3 
On Tue, Jan 02, 2024 at 09:53:30AM +0000, Russell King (Oracle) wrote:
> On Mon, Dec 18, 2023 at 12:50:11AM +0100, Christian Marangi wrote:
> > -	if (qca808x_cdt_fault_length_valid(pair_a))
> > -		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A,
> > -				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A));
> > -	if (qca808x_cdt_fault_length_valid(pair_b))
> > -		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B,
> > -				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B));
> > -	if (qca808x_cdt_fault_length_valid(pair_c))
> > -		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C,
> > -				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C));
> > -	if (qca808x_cdt_fault_length_valid(pair_d))
> > -		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D,
> > -				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D));
> > +	if (qca808x_cdt_fault_length_valid(pair_a)) {
> > +		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A);
> > +		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A, val);
> > +	}
> > +	if (qca808x_cdt_fault_length_valid(pair_b)) {
> > +		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B);
> > +		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B, val);
> > +	}
> > +	if (qca808x_cdt_fault_length_valid(pair_c)) {
> > +		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C);
> > +		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C, val);
> > +	}
> > +	if (qca808x_cdt_fault_length_valid(pair_d)) {
> > +		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D);
> > +		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D, val);
> > +	}
> 
> Maybe a static function for these?
> 
> static void qca808x_get_cdt_length(struct phy_device *phydev, u8 pair)
> {
> 	ethnl_cable_test_fault_length(phydev, pair,
> 				      qca808x_cdt_fault_length(phydev, pair));
> }
> 
> or going via 'val'. Either way, repeating the same two lines multiple
> times seems a bit suboptimal.
> 
> Whether or not you do this:
> 
> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> Thanks!
>

Hi, this has been merged, I will send a followup patch to address this!
(the function has to be touched anyway as it's used by another PHY
Family)
diff mbox series

Patch

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index d5dc927618ab..5e3fbaa7c26c 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -1781,27 +1781,27 @@  static int qca808x_phy_fast_retrain_config(struct phy_device *phydev)
 		return ret;
 
 	phy_write_mmd(phydev, MDIO_MMD_AN, QCA808X_PHY_MMD7_TOP_OPTION1,
-			QCA808X_TOP_OPTION1_DATA);
+		      QCA808X_TOP_OPTION1_DATA);
 	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_20DB,
-			QCA808X_MSE_THRESHOLD_20DB_VALUE);
+		      QCA808X_MSE_THRESHOLD_20DB_VALUE);
 	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_17DB,
-			QCA808X_MSE_THRESHOLD_17DB_VALUE);
+		      QCA808X_MSE_THRESHOLD_17DB_VALUE);
 	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_27DB,
-			QCA808X_MSE_THRESHOLD_27DB_VALUE);
+		      QCA808X_MSE_THRESHOLD_27DB_VALUE);
 	phy_write_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_MSE_THRESHOLD_28DB,
-			QCA808X_MSE_THRESHOLD_28DB_VALUE);
+		      QCA808X_MSE_THRESHOLD_28DB_VALUE);
 	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_1,
-			QCA808X_MMD3_DEBUG_1_VALUE);
+		      QCA808X_MMD3_DEBUG_1_VALUE);
 	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_4,
-			QCA808X_MMD3_DEBUG_4_VALUE);
+		      QCA808X_MMD3_DEBUG_4_VALUE);
 	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_5,
-			QCA808X_MMD3_DEBUG_5_VALUE);
+		      QCA808X_MMD3_DEBUG_5_VALUE);
 	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_3,
-			QCA808X_MMD3_DEBUG_3_VALUE);
+		      QCA808X_MMD3_DEBUG_3_VALUE);
 	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_6,
-			QCA808X_MMD3_DEBUG_6_VALUE);
+		      QCA808X_MMD3_DEBUG_6_VALUE);
 	phy_write_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_DEBUG_2,
-			QCA808X_MMD3_DEBUG_2_VALUE);
+		      QCA808X_MMD3_DEBUG_2_VALUE);
 
 	return 0;
 }
@@ -1838,13 +1838,14 @@  static int qca808x_config_init(struct phy_device *phydev)
 
 	/* Active adc&vga on 802.3az for the link 1000M and 100M */
 	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, QCA808X_PHY_MMD3_ADDR_CLD_CTRL7,
-			QCA808X_8023AZ_AFE_CTRL_MASK, QCA808X_8023AZ_AFE_EN);
+			     QCA808X_8023AZ_AFE_CTRL_MASK, QCA808X_8023AZ_AFE_EN);
 	if (ret)
 		return ret;
 
 	/* Adjust the threshold on 802.3az for the link 1000M */
 	ret = phy_write_mmd(phydev, MDIO_MMD_PCS,
-			QCA808X_PHY_MMD3_AZ_TRAINING_CTRL, QCA808X_MMD3_AZ_TRAINING_VAL);
+			    QCA808X_PHY_MMD3_AZ_TRAINING_CTRL,
+			    QCA808X_MMD3_AZ_TRAINING_VAL);
 	if (ret)
 		return ret;
 
@@ -1870,7 +1871,8 @@  static int qca808x_config_init(struct phy_device *phydev)
 
 	/* Configure adc threshold as 100mv for the link 10M */
 	return at803x_debug_reg_mask(phydev, QCA808X_PHY_DEBUG_ADC_THRESHOLD,
-			QCA808X_ADC_THRESHOLD_MASK, QCA808X_ADC_THRESHOLD_100MV);
+				     QCA808X_ADC_THRESHOLD_MASK,
+				     QCA808X_ADC_THRESHOLD_100MV);
 }
 
 static int qca808x_read_status(struct phy_device *phydev)
@@ -1883,7 +1885,7 @@  static int qca808x_read_status(struct phy_device *phydev)
 		return ret;
 
 	linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->lp_advertising,
-			ret & MDIO_AN_10GBT_STAT_LP2_5G);
+			 ret & MDIO_AN_10GBT_STAT_LP2_5G);
 
 	ret = genphy_read_status(phydev);
 	if (ret)
@@ -2070,18 +2072,22 @@  static int qca808x_cable_test_get_status(struct phy_device *phydev, bool *finish
 	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_D,
 				qca808x_cable_test_result_trans(pair_d));
 
-	if (qca808x_cdt_fault_length_valid(pair_a))
-		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A,
-				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A));
-	if (qca808x_cdt_fault_length_valid(pair_b))
-		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B,
-				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B));
-	if (qca808x_cdt_fault_length_valid(pair_c))
-		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C,
-				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C));
-	if (qca808x_cdt_fault_length_valid(pair_d))
-		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D,
-				qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D));
+	if (qca808x_cdt_fault_length_valid(pair_a)) {
+		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A);
+		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_A, val);
+	}
+	if (qca808x_cdt_fault_length_valid(pair_b)) {
+		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B);
+		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_B, val);
+	}
+	if (qca808x_cdt_fault_length_valid(pair_c)) {
+		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C);
+		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_C, val);
+	}
+	if (qca808x_cdt_fault_length_valid(pair_d)) {
+		val = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D);
+		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_PAIR_D, val);
+	}
 
 	*finished = true;
 
@@ -2148,8 +2154,9 @@  static void qca808x_link_change_notify(struct phy_device *phydev)
 	 * the interface device address is always phy address added by 1.
 	 */
 	mdiobus_c45_modify_changed(phydev->mdio.bus, phydev->mdio.addr + 1,
-			MDIO_MMD_PMAPMD, QCA8081_PHY_SERDES_MMD1_FIFO_CTRL,
-			QCA8081_PHY_FIFO_RSTN, phydev->link ? QCA8081_PHY_FIFO_RSTN : 0);
+				   MDIO_MMD_PMAPMD, QCA8081_PHY_SERDES_MMD1_FIFO_CTRL,
+				   QCA8081_PHY_FIFO_RSTN,
+				   phydev->link ? QCA8081_PHY_FIFO_RSTN : 0);
 }
 
 static struct phy_driver at803x_driver[] = {