Subject: Re: Device tree aware EMAC driver

From: [email-masked] (Tony Breeds)
Date: Wed, 8 Aug 2007 13:16:43 +1000

On Tue, Aug 07, 2007 at 04:22:31PM +1000, David Gibson wrote:
> Based on BenH's earlier work, this is a new version of the EMAC driver
> for the built-in ethernet found on PowerPC 4xx embedded CPUs.  The
> same ASIC is also found in the Axon bridge chip.  This new version is
> designed to work in the arch/powerpc tree, using the device tree to
> probe the device, rather than the old and ugly arch/ppc OCP layer.

<snip>

Hi David,
	I had a look over the patch FWIW, asside from the points below
looks good.

Minor nits:
 * Shouldn't ndev->priv accesses, use netdev_priv() ?  if not a comment
   somewhere about why not will keep the janators off your back.
 * c++ style comments
 * in emac_probe():
	+	/* Wait for dependent devices */
	+	err = -ENODEV;
	+	err = emac_wait_deps(dev);
    The initialisation to -ENODEV is pointless right?
 * s/get_property/of_get_property/g  or you'll make sfr grumpy ;P
 * In drivers/net/ibm_newemac/Makefile, I think the preferred method is
   to use ibm_newemac-y rather than ibm_newemac-objs.

Is there any reason to leave config IBM_NEW_EMAC (and IBM_EMAC) in
drivers/net/Kconfig ?  It seems to me they'd be nicer to in the
appropriate drivers/net/*Kconfig.

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!