Messages in this thread | | | Date | Wed, 29 Nov 2017 22:11:38 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support. |
| |
On Wed, Nov 29, 2017 at 09:37:15PM +0530, Souptick Joarder wrote: > >> +static int bgx_port_sgmii_set_link_speed(struct bgx_port_priv *priv, struct port_status status) > >> +{ > >> + u64 data; > >> + u64 prtx; > >> + u64 miscx; > >> + int timeout; > >> + > > >> + > >> + switch (status.speed) { > >> + case 10: > > > > In my opinion, instead of hard coding the value, is it fine to use ENUM ? > Similar comments applicable in other places where hard coded values are used. >
10 means 10M right? That's not really a magic number. It's fine.
> >> +static int bgx_port_init_xaui_link(struct bgx_port_priv *priv) > >> +{ > > >> + > >> + if (use_ber) { > >> + timeout = 10000; > >> + do { > >> + data = > >> + oct_csr_read(BGX_SPU_BR_STATUS1(priv->node, priv->bgx, priv->index)); > >> + if (data & BIT(0)) > >> + break; > >> + timeout--; > >> + udelay(1); > >> + } while (timeout); > > > > In my opinion, it's better to implement similar kind of loops inside macros.
I don't understand what you mean here. For what it's worth this code seems clear enough to me (except for the bad indenting of oct_csr_read().
It should be something like: data = oct_csr_read(BGX_SPU_BR_STATUS1(priv->node, priv->bgx, priv->index));
That's over the 80 char limit but so is the original code.
regards, dan carpenter
| |