lkml.org 
[lkml]   [2017]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2017-11-29 20:13    [W:2.276 / U:0.308 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site