On Fri, Jul 04, 2025 at 08:13:19PM +0200, Andrew Lunn wrote:
> > #define MBX_FEATURE_WRITE_DELAY BIT(1)
> > u32 mbx_feature;
> > /* cm3 <-> pf mbx */
> > - u32 cpu_pf_shm_base;
> > - u32 pf2cpu_mbox_ctrl;
> > - u32 pf2cpu_mbox_mask;
> > - u32 cpu_pf_mbox_mask;
> > - u32 cpu2pf_mbox_vec;
> > + u32 fw_pf_shm_base;
> > + u32 pf2fw_mbox_ctrl;
> > + u32 pf2fw_mbox_mask;
> > + u32 fw_pf_mbox_mask;
> > + u32 fw2pf_mbox_vec;
>
> Why is a patch adding a new feature deleting code?
>
Not delete code, 'cpu' here means controller in the chip, not host.
So, I just rename 'cpu' to 'fw' to avoid confusion.
> > +/**
> > + * mucse_read_mbx - Reads a message from the mailbox
> > + * @hw: Pointer to the HW structure
> > + * @msg: The message buffer
> > + * @size: Length of buffer
> > + * @mbx_id: Id of vf/fw to read
> > + *
> > + * returns 0 if it successfully read message or else
> > + * MUCSE_ERR_MBX.
> > + **/
> > +s32 mucse_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
>
> s32 is an unusual type for linux. Can the mbox actually return
> negative amounts of data?
>
No, it cann't return negative amounts of data, but this function
returns negative when it failed. Maybe I should use 'int'?
> > +/**
> > + * mucse_write_mbx - Write a message to the mailbox
> > + * @hw: Pointer to the HW structure
> > + * @msg: The message buffer
> > + * @size: Length of buffer
> > + * @mbx_id: Id of vf/fw to write
> > + *
> > + * returns 0 if it successfully write message or else
> > + * MUCSE_ERR_MBX.
>
> Don't invent new error codes. EINVAL would do.
>
Got it, I will fix this.
> > + **/
> > +s32 mucse_write_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> > + enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + s32 ret_val = 0;
> > +
> > + if (size > mbx->size)
> > + ret_val = MUCSE_ERR_MBX;
> > + else if (mbx->ops.write)
> > + ret_val = mbx->ops.write(hw, msg, size, mbx_id);
> > +
> > + return ret_val;
> > +}
> > +static inline void mucse_mbx_inc_pf_ack(struct mucse_hw *hw,
> > + enum MBX_ID mbx_id)
>
> No inline functions in C files. Let the compiler decide.
>
Got it, I will move it to the h file.
> > +static s32 mucse_poll_for_msg(struct mucse_hw *hw, enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + int countdown = mbx->timeout;
> > +
> > + if (!countdown || !mbx->ops.check_for_msg)
> > + goto out;
> > +
> > + while (countdown && mbx->ops.check_for_msg(hw, mbx_id)) {
> > + countdown--;
> > + if (!countdown)
> > + break;
> > + udelay(mbx->usec_delay);
> > + }
> > +out:
> > + return countdown ? 0 : -ETIME;
>
> ETIMEDOUT, not ETIME. Please use iopoll.h, not roll your own.
>
> Andrew
>
> ---
> pw-bot: cr
>
Got it, I will fix it.
Thanks for your feedback.
