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
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
