> > + if (fw_req != 0 && fw_req != hw->mbx.fw_req) {
> > + hw->mbx.stats.reqs++;
> > + return 0;
> > + }
> > +
> > + return -EIO;
>
> Only a suggestion: Might it be clearer to flip the cases and handle the if
> as error case and continue with the success case?
>
> if (fw_req == 0 || fw_req == hw->mbx.fw_req)
> return -EIO;
>
> hw->mbx.stats.reqs++;
> return 0;
That would by the usual pattern in the kernel. It is good to follow
usual patterns.
> > +static void mucse_mbx_inc_pf_req(struct mucse_hw *hw)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + u16 req;
> > + u32 val;
> > +
> > + val = mbx_data_rd32(mbx, MUCSE_MBX_PF2FW_CNT);
> > + req = FIELD_GET(GENMASK_U32(15, 0), val);
>
> Why not assign the values in the declaration like done with mbx?
Reverse Christmas tree.
struct mucse_mbx_info *mbx = &hw->mbx;
u32 val = mbx_data_rd32(mbx, MUCSE_MBX_PF2FW_CNT);
u16 req = FIELD_GET(GENMASK_U32(15, 0), val);
This is not reverse christmas tree. Sometimes you need to put
assignments into the body of the function.
Andrew
