Date:   Fri, 26 Nov 2021 12:09:05 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Heiner Kallweit <hkallweit1@gmail.com>
Cc:     Horatiu Vultur <horatiu.vultur@microchip.com>, andrew@lunn.ch,
        davem@davemloft.net, kuba@kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] net: phy: micrel: Add config_init for LAN8814
Message-ID: <YaDOYc+RWZ35lKjB@shell.armlinux.org.uk>
References: <20211126103833.3609945-1-horatiu.vultur@microchip.com>
 <402780af-9d12-45dd-e435-e7279f1b9263@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <402780af-9d12-45dd-e435-e7279f1b9263@gmail.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Fri, Nov 26, 2021 at 12:57:33PM +0100, Heiner Kallweit wrote:
> Not directly related to just this patch:
> Did you consider implementing the read_page and write_page PHY driver
> callbacks? Then you could use phylib functions like phy_modify_paged et al
> and you wouldn't have to open-code the paged register operations.
> 
> I think write_page would just be
> phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page);
> phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr);
> phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC));
> 
> and read_page
> phy_read(phydev, LAN_EXT_PAGE_ACCESS_CONTROL);

Remember that read_page() and write_page() must be implemented using
the unlocked accessors since the MDIO bus lock is held prior to calling
them. So these should be __phy_write() and __phy_read().

The use of the helpers you mention above also bring greater safety to
the read-modify-write accesses, since with these accessors, the whole
set of accesses are done while holding the bus lock.

Thanks.

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