Re: [PATCH v5 3/5] net: rnpgbe: Add basic mbx ops support

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Aug 21, 2025 at 09:44:11AM +0800, Yibo Dong wrote:
> On Wed, Aug 20, 2025 at 10:23:44PM +0200, Andrew Lunn wrote:
> > > +/**
> > > + * mucse_mbx_get_ack - Read ack from reg
> > > + * @mbx: pointer to the MBX structure
> > > + * @reg: register to read
> > > + *
> > > + * @return: the ack value
> > > + **/
> > > +static u16 mucse_mbx_get_ack(struct mucse_mbx_info *mbx, int reg)
> > > +{
> > > +	return (mbx_data_rd32(mbx, reg) >> 16);
> > > +}
> > 
> > > +static int mucse_check_for_ack_pf(struct mucse_hw *hw)
> > > +{
> > > +	struct mucse_mbx_info *mbx = &hw->mbx;
> > > +	u16 hw_fw_ack;
> > > +
> > > +	hw_fw_ack = mucse_mbx_get_ack(mbx, MBX_FW2PF_COUNTER);
> > 
> > > +int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size)
> > > +{
> > > +	struct mucse_mbx_info *mbx = &hw->mbx;
> > > +	int size_inwords = size / 4;
> > > +	u32 ctrl_reg;
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	ctrl_reg = PF2FW_MBOX_CTRL(mbx);
> > > +	ret = mucse_obtain_mbx_lock_pf(hw);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	for (i = 0; i < size_inwords; i++)
> > > +		mbx_data_wr32(mbx, MBX_FW_PF_SHM_DATA + i * 4, msg[i]);
> > > +
> > > +	/* flush msg and acks as we are overwriting the message buffer */
> > > +	hw->mbx.fw_ack = mucse_mbx_get_ack(mbx, MBX_FW2PF_COUNTER);
> > 
> > It seems like the ACK is always at MBX_FW2PF_COUNTER. So why pass it
> > to mucse_mbx_get_ack()? Please look at your other getters and setters.
> > 
> 
> 'mucse_mbx_get_ack' is always at MBX_FW2PF_COUNTER now, just for pf-fw mbx. 
> But, in the future, there will be pf-vf mbx with different input.
> Should I move 'MBX_FW2PF_COUNTER' to function 'mucse_mbx_get_ack', and
> update the function when I add vf relative code in the future?

Maybe add mucse_mbx_get_pf_ack() so you can later add
mucse_mbx_get_vf_ack()?

The problem is, our crystal ball about what will come next is not very
good. So we review the code we see now, and make comments about it
now. You can add comments explaining why something is the way it is
because in the future it needs to be more generic to handle additional
use cases, etc. Or explain in the commit message.

	Andrew