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: <ZvENzUBT7ni32-Lh@pengutronix.de>
Date: Mon, 23 Sep 2024 08:42:21 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Andrew Lunn <andrew@...n.ch>
Cc: "Alvaro (Al-vuh-roe) Reyes" <a-reyes1@...com>, netdev@...r.kernel.org,
	hkallweit1@...il.com, linux@...linux.org.uk,
	maxime.chevallier@...tlin.com, 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 11:26:49PM +0200, Andrew Lunn wrote:
> 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.

And we will have device-tree based overwrites for the timing role soon,
so there is no reason to store this values in the priv.

Same for RGMII and SGMII modes, DT is already overwriting it with the
phy-mode property.

> > +	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"),
> > +};

I would prefer not to have DP83TG720_PHY_DRIVER() macros. This devices
are not identical. For example: DP83TG721 have HW time stamping,
DP83TG720 don't. As soon as some one will start implementing it, this
macro will be obsolete.

About names: TI DP83TG720CS1.1 and TI DP83TG721CS1.0 are not known
anywhere outside of TI. If you really won't to use this names, please
add comment describing which final products use core variants

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |