Re: [PATCH] Gigabit Ethernet driver of Topcliff PCH

[Sam Ravnborg <sam@ravnborg.org>]

Hi Masayuki Ohtake.

Just a few comments after a very quick scan over some of the files.

	Sam

On Thu, Aug 26, 2010 at 06:56:55PM +0900, Masayuki Ohtake wrote:
> ---

"---" is used to truncate your changelog, random comments goes below "---".
So there would be no changelog entry in this case.


> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 2decc59..fcc6b7b 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -2004,6 +2004,11 @@ menuconfig NETDEV_1000
>  	  If you say N, all options in this submenu will be skipped and disabled.
>  
>  if NETDEV_1000
> +config PCH_GBE
> +        tristate "PCH Gigabit Ethernet"
> +        ---help---
> +          This is an gigabit ethernet driver for PCH.
> +          resources.

Naive readers do not know what PCH is.
Google says it is: Potlatch Corporation

Please elaborate much more.

For new stuff we use "help" - not "---help---".
But you could not know as this is not documented anywhere.

> diff --git a/drivers/net/pch_gbe/Makefile b/drivers/net/pch_gbe/Makefile
> new file mode 100644
> index 0000000..008fa22
> --- /dev/null
> +++ b/drivers/net/pch_gbe/Makefile
> @@ -0,0 +1,6 @@
> +ifeq ($(CONFIG_PCH_GBE_DEBUG_CORE),y)
> +EXTRA_CFLAGS += -DDEBUG
> +endif
Use:
ccflags-$(CONFIG_PCH_GBE_DEBUG_CORE) := -DDEBUG

Btw. where do CONFIG_PCH_GBE_DEBUG_CORE come from. It was not in the Kconfig
file you included.

> +
> +obj-$(CONFIG_PCH_GBE) += pch_gbe.o
> +pch_gbe-objs := pch_gbe_phy.o pch_gbe_ethtool.o pch_gbe_param.o pch_gbe_api.o pch_gbe_main.o

Please do not use -objs.
Do like this:

obj-$(CONFIG_PCH_GBE) += pch_gbe.o
pch_gbe-y :=  pch_gbe_phy.o pch_gbe_ethtool.o pch_gbe_param.o
pch_gbe-y += pch_gbe_api.o pch_gbe_main.o


> diff --git a/drivers/net/pch_gbe/Module.symvers b/drivers/net/pch_gbe/Module.symvers
> new file mode 100644
> index 0000000..e69de29
This file should be dropped.

> diff --git a/drivers/net/pch_gbe/pch_gbe.h b/drivers/net/pch_gbe/pch_gbe.h
> new file mode 100644
> index 0000000..a2b92dd
> --- /dev/null
> +++ b/drivers/net/pch_gbe/pch_gbe.h
> @@ -0,0 +1,425 @@
> +/*
> + * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307, USA.
> + */
> +
> +#ifndef _PCH_GBE_H_
> +#define _PCH_GBE_H_
> +
> +#include <linux/netdevice.h>
> +#include <linux/mii.h>
> +
> +#undef FALSE
> +#define FALSE	0
> +#undef TRUE
> +#define TRUE	1
Use true/false - no reason for these.

> +
> +#ifdef DEBUG
> +#define PCH_GBE_DEBUG(fmt, args...)\
> +	printk(KERN_DEBUG KBUILD_MODNAME ": " fmt, ##args)
> +#else
> +#define PCH_GBE_DEBUG(fmt, args...)	do { } while (0)
> +#endif

Use standard logging methods. 
pr_* and similar.


> +
> +/* Device Driver infomation */
> +#define DRV_STRING      "PCH Network Driver"
> +#define DRV_EXT         "-NAPI"
> +#define DRV_VERSION     "0.95"DRV_EXT
> +#define DRV_DESCRIPTION \
> +	"OKI semiconductor sample Linux driver for PCH Gigabit ethernet"
> +#define DRV_COPYRIGHT   "Copyright(c) 2010 OKI semiconductor"
> +#define FIRM_VERSION    "N/A"
> +/* TX/RX descriptor defines */
> +#define PCH_GBE_MAX_TXD                     4096
> +#define PCH_GBE_MIN_TXD                        8
> +#define PCH_GBE_MAX_RXD                     4096
> +#define PCH_GBE_MIN_RXD                        8
> +/* Number of Transmit and Receive Descriptors must be a multiple of 8 */
> +#define PCH_GBE_TX_DESC_MULTIPLE               8
> +#define PCH_GBE_RX_DESC_MULTIPLE               8

> +/* only works for sizes that are powers of 2 */
> +#define PCH_GBE_ROUNDUP(i, size) ((i) = (((i) + (size) - 1) & ~((size) - 1)))
We have this already. Search include/linux/*

> + * @read_mac_addr:	for pch_gbe_hal_read_mac_addr
> + */
> +struct pch_gbe_functions {
> +	void (*get_bus_info) (struct pch_gbe_hw *);
> +	s32(*init_hw) (struct pch_gbe_hw *);
> +	s32(*read_phy_reg) (struct pch_gbe_hw *, u32, u16 *);
> +	s32(*write_phy_reg) (struct pch_gbe_hw *, u32, u16);
missign space after s32

> +	void (*reset_phy) (struct pch_gbe_hw *);
> +	void (*sw_reset_phy) (struct pch_gbe_hw *);
> +	void (*power_up_phy) (struct pch_gbe_hw *hw);
> +	void (*power_down_phy) (struct pch_gbe_hw *hw);
> +	s32(*read_mac_addr) (struct pch_gbe_hw *);
> +};
> +



> +	printk(KERN_ERR KBUILD_MODNAME ": " "ERROR: Registers not mapped\n");

In several places...
Use standard logging methods to do this simpler.


> +++ b/drivers/net/pch_gbe/pch_gbe_api.h
> @@ -0,0 +1,31 @@
> +/*

Why is two .h files needed for a single driver?
As this file live in drivers/net/pch_gbe/ this is purely internal stuff.

> +#include "pch_gbe_regs.h"
> +#include "pch_gbe.h"
> +#include "pch_gbe_api.h"

As per comment above I realise that this single driver has three .h files.
Maybe their size warrants this?