Re: [PATCH net-next v12 3/5] net: rnpgbe: Add basic mbx ops support

Andrew Lunn
2025-09-16T19:34:19Z

> > + 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