Re: [net-next PATCH v7 5/6] net: phy: Add support for Aeonsemi AS21xxx PHYs

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

On Tue, Apr 15, 2025 at 01:24:16PM +0200, Christian Marangi wrote:
> On Tue, Apr 15, 2025 at 10:37:49AM +0100, Russell King (Oracle) wrote:
> > > +static int aeon_ipcs_wait_cmd(struct phy_device *phydev, bool parity_status)
> > > +{
> > > +	u16 val;
> > > +
> > > +	/* Exit condition logic:
> > > +	 * - Wait for parity bit equal
> > > +	 * - Wait for status success, error OR ready
> > > +	 */
> > > +	return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, VEND1_IPC_STS, val,
> > > +					 FIELD_GET(AEON_IPC_STS_PARITY, val) == parity_status &&
> > > +					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_RCVD &&
> > > +					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_PROCESS &&
> > > +					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_BUSY,
> > > +					 AEON_IPC_DELAY, AEON_IPC_TIMEOUT, false);
> > 
> > Hmm. I'm wondering whether:
> > 
> > static bool aeon_ipc_ready(u16 val, bool parity_status)
> > {
> > 	u16 status;
> > 
> > 	if (FIELD_GET(AEON_IPC_STS_PARITY, val) != parity_status)
> > 		return false;
> > 
> > 	status = val & AEON_IPC_STS_STATUS;
> > 
> > 	return status != AEON_IPC_STS_STATUS_RCVD &&
> > 	       status != AEON_IPC_STS_STATUS_PROCESS &&
> > 	       status != AEON_IPC_STS_STATUS_BUSY;
> > }
> > 
> > would be better, and then maybe you can fit the code into less than 80
> > columns. I'm not a fan of FIELD_PREP_CONST() when it causes differing
> > usage patterns like the above (FIELD_GET(AEON_IPC_STS_STATUS, val)
> > would match the coding style, and probably makes no difference to the
> > code emitted.)
> > 
> 
> You are suggesting to use a generic readx function or use a while +
> sleep to use the suggested _ready function?

To write the other part of it (I thought this would be obvious!):

+	return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, VEND1_IPC_STS, val,
+					 aeon_ipc_ready(val, parity_status),
+					 AEON_IPC_DELAY, AEON_IPC_TIMEOUT, false);

> Mhhh I think I will have to create __ function for locked and non-locked
> variant. I think I woulkd just handle the lock in the function using
> send and rcv and maybe add some smatch tag to make sure the lock is
> taken when entering those functions.

If you don't need the receive part, then pass NULL in for the receive
data pointer, and use that to conditionalise that part?

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