On Tue, Dec 10, 2024 at 02:53:34AM +0100, Andrew Lunn wrote:

quoted

+static int an855_phy_restore_page(struct an8855_mfd_priv *priv,
+				  int phy) __must_hold(&priv->bus->mdio_lock)
+{
+	/* Check PHY page only for addr shared with switch */
+	if (phy != priv->switch_addr)
+		return 0;
+
+	/* Don't restore page if it's not set to switch page */
+	if (priv->current_page != FIELD_GET(AN8855_PHY_PAGE,
+					    AN8855_PHY_PAGE_EXTENDED_4))
+		return 0;
+
+	/* Restore page to 0, PHY might change page right after but that
+	 * will be ignored as it won't be a switch page.
+	 */
+	return an8855_mii_set_page(priv, phy, AN8855_PHY_PAGE_STANDARD);
+}

I don't really understand what is going on here. Maybe the commit
message needs expanding, or the function names changing.

Generally, i would expect a save/restore action. Save the current
page, swap to the PHY page, do the PHY access, and then restore to the
saved page.

Idea is to save on extra read/write on subsequent write on the same
page.

Idea here is that PHY will receive most of the read (for status
poll) hence in 90% of the time page will be 0.

And switch will receive read/write only on setup or fdb/vlan access on
configuration so it will receive subsequent write on the same page.
(page 4)

PHY might also need to write on page 1 on setup but never on page 4 as
that is reserved for switch.

Making the read/swap/write/restore adds 2 additional operation that can
really be skipped 90% of the time.

Also curret_page cache is indirectly protected by the mdio lock.

So in short this function makes sure PHY for port 0 is configured on the
right page based on the prev page set.

An alternative way might be assume PHY is always on page 0 and any
switch operation save and restore the page.

Hope it's clear now why this is needed. Is this ok or you prefer the
alternative way? 

-- 
	Ansuel