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

[Andrew Lunn <andrew@lunn.ch>]

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