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

[Christian Marangi <ansuelsmth@gmail.com>]

On Tue, Apr 15, 2025 at 01:55:29PM +0100, Russell King (Oracle) wrote:
> 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);
>

Eh I never considered that I could totally pass entire function in the
condition part. 

> > 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?
> 

Ok yes that can also work.

-- 
	Ansuel