On Thu, Aug 21, 2025 at 05:06:50AM +0200, Andrew Lunn wrote:
> 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
>
Got it, I will move MBX_FW2PF_COUNTER to mucse_mbx_get_ack and rename
the fucntion. And add new function later.
Thanks for your feedback.
Thanks for your feedback.
