diff mbox series

[net-next,08/14] net: phy: at803x: drop specific PHY id check from cable test functions

Message ID 20231129021219.20914-9-ansuelsmth@gmail.com (mailing list archive)
State Not Applicable
Headers show
Series net: phy: at803x: cleanup + split | expand

Commit Message

Christian Marangi Nov. 29, 2023, 2:12 a.m. UTC 
Drop specific PHY id check for cable test functions for at803x. This is
done to make functions more generic.

PHYs that requires to set additional reg are moved to specific function
calling the more generic one.

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

Comments

Russell King (Oracle) Nov. 29, 2023, 9:38 a.m. UTC | #1 
On Wed, Nov 29, 2023 at 03:12:13AM +0100, Christian Marangi wrote:
> @@ -1310,10 +1302,6 @@ static int at803x_cable_test_start(struct phy_device *phydev)
>  	 */
>  	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
>  	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
> -	if (phydev->phy_id != ATH9331_PHY_ID &&
> -	    phydev->phy_id != ATH8032_PHY_ID &&
> -	    phydev->phy_id != QCA9561_PHY_ID)
> -		phy_write(phydev, MII_CTRL1000, 0);
...
> +static int at8031_cable_test_start(struct phy_device *phydev)
> +{
> +	at803x_cable_test_start(phydev);
> +	phy_write(phydev, MII_CTRL1000, 0);

I don't think this is a safe change - same reasons as given on a
previous patch. You can't randomly reorder register writes like this.
Christian Marangi Nov. 29, 2023, 9:47 a.m. UTC | #2 
On Wed, Nov 29, 2023 at 09:38:39AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 29, 2023 at 03:12:13AM +0100, Christian Marangi wrote:
> > @@ -1310,10 +1302,6 @@ static int at803x_cable_test_start(struct phy_device *phydev)
> >  	 */
> >  	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
> >  	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
> > -	if (phydev->phy_id != ATH9331_PHY_ID &&
> > -	    phydev->phy_id != ATH8032_PHY_ID &&
> > -	    phydev->phy_id != QCA9561_PHY_ID)
> > -		phy_write(phydev, MII_CTRL1000, 0);
> ...
> > +static int at8031_cable_test_start(struct phy_device *phydev)
> > +{
> > +	at803x_cable_test_start(phydev);
> > +	phy_write(phydev, MII_CTRL1000, 0);
> 
> I don't think this is a safe change - same reasons as given on a
> previous patch. You can't randomly reorder register writes like this.
>

Actually for this the order is keeped. Generic function is called and
for at8031 MII_CTRL1000 is called on top of that.
Russell King (Oracle) Nov. 29, 2023, 10:57 a.m. UTC | #3 
On Wed, Nov 29, 2023 at 10:47:18AM +0100, Christian Marangi wrote:
> On Wed, Nov 29, 2023 at 09:38:39AM +0000, Russell King (Oracle) wrote:
> > On Wed, Nov 29, 2023 at 03:12:13AM +0100, Christian Marangi wrote:
> > > @@ -1310,10 +1302,6 @@ static int at803x_cable_test_start(struct phy_device *phydev)
> > >  	 */
> > >  	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
> > >  	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
> > > -	if (phydev->phy_id != ATH9331_PHY_ID &&
> > > -	    phydev->phy_id != ATH8032_PHY_ID &&
> > > -	    phydev->phy_id != QCA9561_PHY_ID)
> > > -		phy_write(phydev, MII_CTRL1000, 0);
> > ...
> > > +static int at8031_cable_test_start(struct phy_device *phydev)
> > > +{
> > > +	at803x_cable_test_start(phydev);
> > > +	phy_write(phydev, MII_CTRL1000, 0);
> > 
> > I don't think this is a safe change - same reasons as given on a
> > previous patch. You can't randomly reorder register writes like this.
> >
> 
> Actually for this the order is keeped. Generic function is called and
> for at8031 MII_CTRL1000 is called on top of that.

Okay, but I don't like it. I would prefer this to be:

static void at803x_cable_test_autoneg(struct phy_device *phydev)
{
	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
}

static int at803x_cable_test_start(struct phy_device *phydev)
{
	at803x_cable_test_autoneg(phydev);
	return 0;
}

static int at8031_cable_test_start(struct phy_device *phydev)
{
	at803x_cable_test_autoneg(phydev);
	phy_write(phydev, MII_CTRL1000, 0);
	return 0;
}

which makes it more explicit what is going on here. Also a comment
above the function stating that it's for AR8031 _and_ AR8035 would
be useful.
Christian Marangi Nov. 29, 2023, 11:04 a.m. UTC | #4 
On Wed, Nov 29, 2023 at 10:57:28AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 29, 2023 at 10:47:18AM +0100, Christian Marangi wrote:
> > On Wed, Nov 29, 2023 at 09:38:39AM +0000, Russell King (Oracle) wrote:
> > > On Wed, Nov 29, 2023 at 03:12:13AM +0100, Christian Marangi wrote:
> > > > @@ -1310,10 +1302,6 @@ static int at803x_cable_test_start(struct phy_device *phydev)
> > > >  	 */
> > > >  	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
> > > >  	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
> > > > -	if (phydev->phy_id != ATH9331_PHY_ID &&
> > > > -	    phydev->phy_id != ATH8032_PHY_ID &&
> > > > -	    phydev->phy_id != QCA9561_PHY_ID)
> > > > -		phy_write(phydev, MII_CTRL1000, 0);
> > > ...
> > > > +static int at8031_cable_test_start(struct phy_device *phydev)
> > > > +{
> > > > +	at803x_cable_test_start(phydev);
> > > > +	phy_write(phydev, MII_CTRL1000, 0);
> > > 
> > > I don't think this is a safe change - same reasons as given on a
> > > previous patch. You can't randomly reorder register writes like this.
> > >
> > 
> > Actually for this the order is keeped. Generic function is called and
> > for at8031 MII_CTRL1000 is called on top of that.
> 
> Okay, but I don't like it. I would prefer this to be:
> 
> static void at803x_cable_test_autoneg(struct phy_device *phydev)
> {
> 	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
> 	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
> }
> 
> static int at803x_cable_test_start(struct phy_device *phydev)
> {
> 	at803x_cable_test_autoneg(phydev);
> 	return 0;
> }
> 
> static int at8031_cable_test_start(struct phy_device *phydev)
> {
> 	at803x_cable_test_autoneg(phydev);
> 	phy_write(phydev, MII_CTRL1000, 0);
> 	return 0;
> }
> 
> which makes it more explicit what is going on here. Also a comment
> above the function stating that it's for AR8031 _and_ AR8035 would
> be useful.
>

Much cleaner thanks for the hint!
Russell King (Oracle) Nov. 29, 2023, 11:07 a.m. UTC | #5 
Andrew,

On Wed, Nov 29, 2023 at 03:12:13AM +0100, Christian Marangi wrote:
>  static int at8035_parse_dt(struct phy_device *phydev)
>  {
>  	struct device_node *node = phydev->mdio.dev.of_node;
> @@ -2205,8 +2213,8 @@ static struct phy_driver at803x_driver[] = {
>  	.handle_interrupt	= at803x_handle_interrupt,
>  	.get_tunable		= at803x_get_tunable,
>  	.set_tunable		= at803x_set_tunable,
> -	.cable_test_start	= at803x_cable_test_start,
> -	.cable_test_get_status	= at803x_cable_test_get_status,
> +	.cable_test_start	= at8031_cable_test_start,
> +	.cable_test_get_status	= at8031_cable_test_get_status,
>  }, {
>  	/* Qualcomm Atheros AR8030 */
>  	.phy_id			= ATH8030_PHY_ID,
> @@ -2243,8 +2251,8 @@ static struct phy_driver at803x_driver[] = {
>  	.handle_interrupt	= at803x_handle_interrupt,
>  	.get_tunable		= at803x_get_tunable,
>  	.set_tunable		= at803x_set_tunable,
> -	.cable_test_start	= at803x_cable_test_start,
> -	.cable_test_get_status	= at803x_cable_test_get_status,
> +	.cable_test_start	= at8031_cable_test_start,
> +	.cable_test_get_status	= at8031_cable_test_get_status,
>  }, {
>  	/* Qualcomm Atheros AR8032 */
>  	PHY_ID_MATCH_EXACT(ATH8032_PHY_ID),
> @@ -2259,7 +2267,7 @@ static struct phy_driver at803x_driver[] = {
>  	.config_intr		= at803x_config_intr,
>  	.handle_interrupt	= at803x_handle_interrupt,
>  	.cable_test_start	= at803x_cable_test_start,
> -	.cable_test_get_status	= at803x_cable_test_get_status,
> +	.cable_test_get_status	= at8032_cable_test_get_status,
>  }, {
>  	/* ATHEROS AR9331 */
>  	PHY_ID_MATCH_EXACT(ATH9331_PHY_ID),
> @@ -2272,7 +2280,7 @@ static struct phy_driver at803x_driver[] = {
>  	.config_intr		= at803x_config_intr,
>  	.handle_interrupt	= at803x_handle_interrupt,
>  	.cable_test_start	= at803x_cable_test_start,
> -	.cable_test_get_status	= at803x_cable_test_get_status,
> +	.cable_test_get_status	= at8032_cable_test_get_status,
>  	.read_status		= at803x_read_status,
>  	.soft_reset		= genphy_soft_reset,
>  	.config_aneg		= at803x_config_aneg,
> @@ -2288,7 +2296,7 @@ static struct phy_driver at803x_driver[] = {
>  	.config_intr		= at803x_config_intr,
>  	.handle_interrupt	= at803x_handle_interrupt,
>  	.cable_test_start	= at803x_cable_test_start,
> -	.cable_test_get_status	= at803x_cable_test_get_status,
> +	.cable_test_get_status	= at8032_cable_test_get_status,
>  	.read_status		= at803x_read_status,
>  	.soft_reset		= genphy_soft_reset,
>  	.config_aneg		= at803x_config_aneg,

We could _really_ do with moving away from an array of PHY driver
structures in phylib because patches like this are hard to properly
review. The problem is there is little context to say _which_ driver
instance is being changed. The only thing that saves us above are
the comments on the next instance - but those may not be present
if we're modifying something in the middle of each definition.

The same issue happens with the mv88e6xxx driver, with that big
array in chip.c, where we have loads of function pointers. It's
far from ideal.

Maybe we should consider moving to a model where each driver is
defined as a separate named structure, and then we have an array
of pointers to each driver, which is then passed into a new PHY
driver registration function? This way, at least the @@ line will
identify to a reviewer which instance is being modified.

This won't help the problem of a patch being mis-applied due to
there not being sufficient differences in context, but if one
subsequently diffs after applying such a change and compares the
patch to the original, there will be a difference in the @@ line.
(However, arguably that level of checking is unlikely to happen.)
diff mbox series

Patch

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index e7d006ca1676..8f5878ccb1a8 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -1263,19 +1263,11 @@  static int at803x_cable_test_one_pair(struct phy_device *phydev, int pair)
 }
 
 static int at803x_cable_test_get_status(struct phy_device *phydev,
-					bool *finished)
+					bool *finished, unsigned long pair_mask)
 {
-	unsigned long pair_mask;
 	int retries = 20;
 	int pair, ret;
 
-	if (phydev->phy_id == ATH9331_PHY_ID ||
-	    phydev->phy_id == ATH8032_PHY_ID ||
-	    phydev->phy_id == QCA9561_PHY_ID)
-		pair_mask = 0x3;
-	else
-		pair_mask = 0xf;
-
 	*finished = false;
 
 	/* According to the datasheet the CDT can be performed when
@@ -1310,10 +1302,6 @@  static int at803x_cable_test_start(struct phy_device *phydev)
 	 */
 	phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
 	phy_write(phydev, MII_ADVERTISE, ADVERTISE_CSMA);
-	if (phydev->phy_id != ATH9331_PHY_ID &&
-	    phydev->phy_id != ATH8032_PHY_ID &&
-	    phydev->phy_id != QCA9561_PHY_ID)
-		phy_write(phydev, MII_CTRL1000, 0);
 
 	/* we do all the (time consuming) work later */
 	return 0;
@@ -1664,6 +1652,26 @@  static int at8031_set_wol(struct phy_device *phydev,
 	return at803x_set_wol(phydev, wol);
 }
 
+static int at8031_cable_test_get_status(struct phy_device *phydev,
+					bool *finished)
+{
+	return at803x_cable_test_get_status(phydev, finished, 0xf);
+}
+
+static int at8031_cable_test_start(struct phy_device *phydev)
+{
+	at803x_cable_test_start(phydev);
+	phy_write(phydev, MII_CTRL1000, 0);
+
+	return 0;
+}
+
+static int at8032_cable_test_get_status(struct phy_device *phydev,
+					bool *finished)
+{
+	return at803x_cable_test_get_status(phydev, finished, 0x3);
+}
+
 static int at8035_parse_dt(struct phy_device *phydev)
 {
 	struct device_node *node = phydev->mdio.dev.of_node;
@@ -2205,8 +2213,8 @@  static struct phy_driver at803x_driver[] = {
 	.handle_interrupt	= at803x_handle_interrupt,
 	.get_tunable		= at803x_get_tunable,
 	.set_tunable		= at803x_set_tunable,
-	.cable_test_start	= at803x_cable_test_start,
-	.cable_test_get_status	= at803x_cable_test_get_status,
+	.cable_test_start	= at8031_cable_test_start,
+	.cable_test_get_status	= at8031_cable_test_get_status,
 }, {
 	/* Qualcomm Atheros AR8030 */
 	.phy_id			= ATH8030_PHY_ID,
@@ -2243,8 +2251,8 @@  static struct phy_driver at803x_driver[] = {
 	.handle_interrupt	= at803x_handle_interrupt,
 	.get_tunable		= at803x_get_tunable,
 	.set_tunable		= at803x_set_tunable,
-	.cable_test_start	= at803x_cable_test_start,
-	.cable_test_get_status	= at803x_cable_test_get_status,
+	.cable_test_start	= at8031_cable_test_start,
+	.cable_test_get_status	= at8031_cable_test_get_status,
 }, {
 	/* Qualcomm Atheros AR8032 */
 	PHY_ID_MATCH_EXACT(ATH8032_PHY_ID),
@@ -2259,7 +2267,7 @@  static struct phy_driver at803x_driver[] = {
 	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.cable_test_start	= at803x_cable_test_start,
-	.cable_test_get_status	= at803x_cable_test_get_status,
+	.cable_test_get_status	= at8032_cable_test_get_status,
 }, {
 	/* ATHEROS AR9331 */
 	PHY_ID_MATCH_EXACT(ATH9331_PHY_ID),
@@ -2272,7 +2280,7 @@  static struct phy_driver at803x_driver[] = {
 	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.cable_test_start	= at803x_cable_test_start,
-	.cable_test_get_status	= at803x_cable_test_get_status,
+	.cable_test_get_status	= at8032_cable_test_get_status,
 	.read_status		= at803x_read_status,
 	.soft_reset		= genphy_soft_reset,
 	.config_aneg		= at803x_config_aneg,
@@ -2288,7 +2296,7 @@  static struct phy_driver at803x_driver[] = {
 	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
 	.cable_test_start	= at803x_cable_test_start,
-	.cable_test_get_status	= at803x_cable_test_get_status,
+	.cable_test_get_status	= at8032_cable_test_get_status,
 	.read_status		= at803x_read_status,
 	.soft_reset		= genphy_soft_reset,
 	.config_aneg		= at803x_config_aneg,