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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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.)


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!