linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Jens Osterkamp <jens@de.ibm.com>
To: Francois Romieu <romieu@fr.zoreil.com>
Cc: jgarzik@pobox.com, James K Lewis <jim@jklewis.com>,
	linuxppc-dev@ozlabs.org, netdev@vger.kernel.org,
	cbe-oss-dev@ozlabs.org
Subject: Re: spidernet: add improved phy support in sungem_phy.c
Date: Thu, 1 Feb 2007 11:54:49 +0100	[thread overview]
Message-ID: <200702011154.49558.jens@de.ibm.com> (raw)
In-Reply-To: <20070126233809.GA16660@electric-eye.fr.zoreil.com>


Francois,

thank you for your comments. I'll send a revised patch soon.

> +#define BCM5421_MODE_MASK	1 << 5
> 
> Please add parenthesis.

Done.

> "&" is fine despite the lack of parenthesis above but it is error-prone.

Corrected.

> 
> +
> +	if ( mode == GMII_COPPER) {
>            ^^^
> +		return genmii_poll_link(phy);
> +	}
> 
> No curly-braces for single line statements please.

I corrected this on all my additions.

> Ternary operator ?

I dont like ternary operators. Yes, I know, they are cool, but
they make the code much less readable IMHO.

> +#define BCM5461_FIBER_LINK	1 << 2
> +#define BCM5461_MODE_MASK	3 << 1
> 
> Please add parenthesis.

Done.

> Join the dark side and use a ternary operator.

See above.

> +static int bcm5461_enable_fiber(struct mii_phy* phy, int autoneg)
> +{
> +	/*
> +	phy_write(phy, MII_NCONFIG, 0xfc0c);
> +	phy_write(phy, MII_BMCR, 0x4140);
> +	*/
> 
> Remove ?

Done.

Jens

      parent reply	other threads:[~2007-02-01 10:54 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-01-26 13:07 spidernet: add improved phy support in sungem_phy.c Jens Osterkamp
2007-01-26 20:20 ` Benjamin Herrenschmidt
2007-01-26 22:31   ` Jens Osterkamp
2007-01-26 23:38     ` Francois Romieu
2007-01-30 22:30       ` Linas Vepstas
2007-02-01 10:55         ` Jens Osterkamp
2007-02-01 10:57         ` [PATCH] [v2] spidernet: add improved phy support Jens Osterkamp
2007-02-01 10:54       ` Jens Osterkamp [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200702011154.49558.jens@de.ibm.com \
    --to=jens@de.ibm.com \
    --cc=cbe-oss-dev@ozlabs.org \
    --cc=jgarzik@pobox.com \
    --cc=jim@jklewis.com \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=netdev@vger.kernel.org \
    --cc=romieu@fr.zoreil.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).