Received-SPF: pass (google.com: domain of linux-kernel+bounces-6743-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <30673ad6023785016bddec34eb97f4259580340a.camel@perches.com>
Subject: Re: [net-next PATCH v2] net: phy: at803x: better align function
 varibles to open parenthesis
From: Joe Perches <joe@perches.com>
To: Christian Marangi <ansuelsmth@gmail.com>, Andrew Lunn <andrew@lunn.ch>, 
 Heiner Kallweit <hkallweit1@gmail.com>, Russell King
 <linux@armlinux.org.uk>, "David S. Miller" <davem@davemloft.net>, Eric
 Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo
 Abeni <pabeni@redhat.com>, netdev@vger.kernel.org, 
 linux-kernel@vger.kernel.org
Date: Wed, 20 Dec 2023 02:28:34 -0800
In-Reply-To: <20231219202124.30013-1-ansuelsmth@gmail.com>
References: <20231219202124.30013-1-ansuelsmth@gmail.com>
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
Precedence: bulk
MIME-Version: 1.0

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.
>=20
> 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 p=
hy_device *phydev, bool *finish
>  	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_D,
>  				qca808x_cable_test_result_trans(pair_d));
> =20
> -	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 =3D 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 =3D 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 =3D 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 =3D 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 =3D 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);