netdev - Re: [PATCH 3/5] net: phy: dp83tg720: Extending support to DP83TG721 PHY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0092fd9d-22e4-458a-8227-618fc56f5459@lunn.ch>
Date: Thu, 19 Sep 2024 23:26:49 +0200
From: Andrew Lunn <andrew@...n.ch>
To: "Alvaro (Al-vuh-roe) Reyes" <a-reyes1@...com>
Cc: netdev@...r.kernel.org, hkallweit1@...il.com, linux@...linux.org.uk,
	maxime.chevallier@...tlin.com, o.rempel@...gutronix.de,
	spatton@...com, r-kommineni@...com, e-mayhew@...com,
	praneeth@...com, p-varis@...com, d-qiu@...com
Subject: Re: [PATCH 3/5] net: phy: dp83tg720: Extending support to DP83TG721
 PHY

On Thu, Sep 19, 2024 at 02:01:17PM -0700, Alvaro (Al-vuh-roe) Reyes wrote:
> The DP83TG721 is the next revision of the DP83TG720 and will share the
> same driver. Added PHY_ID and probe funtion to check which version is
> being loaded. 

Please don't mix whitespace changes with real code changes. It makes
it harder to see the real changes which need reviewing.

> +enum DP83TG720_chip_type {
> +	DP83TG720_CS1_1,
> +	DP83TG721_CS1,
> +};
> +
> +struct DP83TG720_private {
> +	int chip;

I _think_ this should be enum DP83TG720_chip_type chip;

> +	bool is_master;

I think this can be removed and replaced with
phydev->master_slave_set. You probably want to mirror it into
phydev->master_slave_get.

phydev->master_slave_state normally contains the result of auto-neg,
but i expect this PHY forces it, so again, you probably want to mirror
it here as well. Test it out with ethtool and make sure it reports
what you expect.

> +	struct DP83TG720_private *DP83TG720;

Upper case is very unusual in mainline. It is generally only used for
CPP macros.

> +static struct phy_driver dp83tg720_driver[] = {
> +    DP83TG720_PHY_DRIVER(DP83TG720_CS_1_1_PHY_ID, "TI DP83TG720CS1.1"),
> +	DP83TG720_PHY_DRIVER(DP83TG721_CS_1_0_PHY_ID, "TI DP83TG721CS1.0"),
> +};

Indentation is messed up here. I expect checkpatch says something
about this?

>  module_phy_driver(dp83tg720_driver);
>  
>  static struct mdio_device_id __maybe_unused dp83tg720_tbl[] = {
> -	{ PHY_ID_MATCH_MODEL(DP83TG720_PHY_ID) },
> -	{ }
> +    { PHY_ID_MATCH_EXACT(DP83TG720_CS_1_1_PHY_ID) },
> +	{ PHY_ID_MATCH_EXACT(DP83TG721_CS_1_0_PHY_ID) },
> +	{ },

Here as well.

>  };
>  MODULE_DEVICE_TABLE(mdio, dp83tg720_tbl);
>  
> +// static struct phy_driver dp83tg720_driver[] = {
> +// {
> +// 	PHY_ID_MATCH_MODEL(DP83TG720_PHY_ID),
> +// 	.name		= "TI DP83TG720",
> +
> +// 	.flags          = PHY_POLL_CABLE_TEST,
> +// 	.config_aneg	= dp83tg720_config_aneg,
> +// 	.read_status	= dp83tg720_read_status,
> +// 	.get_features	= genphy_c45_pma_read_ext_abilities,
> +// 	.config_init	= dp83tg720_config_init,
> +// 	.get_sqi	= dp83tg720_get_sqi,
> +// 	.get_sqi_max	= dp83tg720_get_sqi_max,
> +// 	.cable_test_start = dp83tg720_cable_test_start,
> +// 	.cable_test_get_status = dp83tg720_cable_test_get_status,
> +
> +// 	.suspend	= genphy_suspend,
> +// 	.resume		= genphy_resume,
> +// } };
> +// module_phy_driver(dp83tg720_driver);
> +
> +// static struct mdio_device_id __maybe_unused dp83tg720_tbl[] = {
> +// 	{ PHY_ID_MATCH_MODEL(DP83TG720_PHY_ID) },
> +// 	{ }
> +// };
> +// MODULE_DEVICE_TABLE(mdio, dp83tg720_tbl);

Please don't add code which is commented out.


    Andrew

---
pw-bot: cr