Subject: Re: [PATCH] Gigabit Ethernet driver of Topcliff PCH

From: "Masayuki Ohtake" <[email-masked]>
Date: Thu, 26 Aug 2010 21:47:46 +0900

Hi Sam

Thank you for your comment.

The reply's comment was buried in following.
I will modify this patch.

Thanks, Ohtake(OKISEMI)
----- Original Message ----- 
From: "Sam Ravnborg" <[email-masked]>
To: "Masayuki Ohtake" <[email-masked]>
Cc: "LKML" <[email-masked]>; "ML netdev" <[email-masked]>; "Greg Rose" <[email-masked]>;
"Maxime Bizon" <[email-masked]>; "Kristoffer Glembo" <[email-masked]>; "Ralf Baechle" <[email-masked]>;
"John Linn" <[email-masked]>; "Randy Dunlap" <[email-masked]>; "David S. Miller" <[email-masked]>;
"MeeGo" <[email-masked]>; "Wang, Qi" <[email-masked]>; "Wang, Yong Y" <[email-masked]>; "Andrew"
<[email-masked]>; "Intel OTC" <[email-masked]>; "Foster, Margie" <[email-masked]>;
"Toshiharu Okada" <[email-masked]>; "Tomoya Morinaga" <[email-masked]>; "Takahiro Shimizu"
<[email-masked]>
Sent: Thursday, August 26, 2010 7:28 PM
Subject: Re: [PATCH] Gigabit Ethernet driver of Topcliff PCH

> 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.

<masa>
OK

>
> > 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.

<masa>
Topcliff PCH is the platform controller hub that is going to be used in
Intel's upcoming general embedded platform.

Explanation is added to help.

>
> > 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.
>

<masa>
This deletes.
Since a standard logging is used

> > +
> > +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
>

<masa>
This modifies as above.

>
> > 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.

<masa>
This modifies

>
> > 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.
>

<masa>
This modifies

> > +
> > +#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.
>

<masa>
This modifies

>
> > +
> > +/* 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/*
>

<masa>
This modifies

> > + * @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
>

<masa>
This modifies

> > + 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.

<masa>
This modifies

>
>
> > +++ 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.

<masa>
This driver is designed be easy to add other PHY.
The api.c/h wraps the interface of PHY.

>
> > +#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?
>

<masa>
"pch_gbe_regs.h" shows the register of MAC.
"pch_gbe_regs.h" is merged into "pch_gbe.h".