On Wed, Aug 13, 2025 at 02:33:39PM +0100, Vadim Fedorenko wrote:
> On 13/08/2025 10:52, Yibo Dong wrote:
> > On Tue, Aug 12, 2025 at 05:14:15PM +0100, Vadim Fedorenko wrote:
> > > On 12/08/2025 10:39, Dong Yibo wrote:
> > > > Initialize basic mbx_fw ops, such as get_capability, reset phy
> > > > and so on.
> > > >
> > > > Signed-off-by: Dong Yibo <dong100@xxxxxxxxx>
> > > > +static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
> > > > + struct mbx_fw_cmd_req *req,
> > > > + struct mbx_fw_cmd_reply *reply)
> > > > +{
> > > > + int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
> > > > + int retry_cnt = 3;
> > > > + int err;
> > > > +
> > > > + err = mutex_lock_interruptible(&hw->mbx.lock);
> > > > + if (err)
> > > > + return err;
> > > > + err = hw->mbx.ops->write_posted(hw, (u32 *)req,
> > > > + L_WD(len));
> > > > + if (err) {> + mutex_unlock(&hw->mbx.lock);
> > > > + return err;
> > > > + }
> > >
> > > it might look a bit cleaner if you add error label and have unlock code
> > > once in the end of the function...
> > >
> >
> > If it is more cleaner bellow?
> >
> > static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
> > struct mbx_fw_cmd_req *req,
> > struct mbx_fw_cmd_reply *reply)
> > {
> > int len = le16_to_cpu(req->datalen) + MBX_REQ_HDR_LEN;
> > int retry_cnt = 3;
> > int err;
> >
> > err = mutex_lock_interruptible(&hw->mbx.lock);
> > if (err)
> > return err;
> > err = hw->mbx.ops->write_posted(hw, (u32 *)req,
> > L_WD(len));
> > if (err)
> > goto quit;
> > do {
> > err = hw->mbx.ops->read_posted(hw, (u32 *)reply,
> > L_WD(sizeof(*reply)));
> > if (err)
> > goto quit;
> > } while (--retry_cnt >= 0 && reply->opcode != req->opcode);
> >
> > mutex_unlock(&hw->mbx.lock);
> > if (retry_cnt < 0)
> > return -ETIMEDOUT;
> > if (reply->error_code)
> > return -EIO;
> > return 0;
> > quit:
> > mutex_unlock(&hw->mbx.lock);
> > return err;
> > }
> >
>
> Maybe:
>
> } while (--retry_cnt >= 0 && reply->opcode != req->opcode);
>
> quit:
> mutex_unlock(&hw->mbx.lock);
> if (!err && retry_cnt < 0)
> return -ETIMEDOUT;
> if (!err && reply->error_code)
> return -EIO;
> return err;
>
>
> looks cleaner
>
>
Got it, I will update this, thanks.
