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

[Joe Perches <joe@perches.com>]

On Tue, 2023-12-19 at 21:21 +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.
[]
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
[]
> @@ -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);
> +	}

trivia:

Using the preprocessor can make this look simpler so there's
no wall of text and no duplicated identifiers.

Something like:

#define test_length(phydev, locase, upcase)						\
	if (qca808x_cdt_fault_length(locase)) {						\
		int len = qca808x_cdt_fault_length(phydev, ETHTOOL_A_CABLE_ ## upcase);	\
											\
		ethnl_cable_test_fault_length(phydev, ETHTOOL_A_CABLE_ ## upcase, len);	\
	}

so these could be

	test_length(phydev, pair_a, PAIR_A);
	test_length(phydev, pair_b, PAIR_B);
	test_length(phydev, pair_d, PAIR_C);
	test_length(phydev, pair_d, PAIR_D);