[PATCH V2 net-next 0/2] pch_gbe: Add MinnowBoard support

[PATCH V2 net-next 0/2] pch_gbe: Add MinnowBoard support

[Darren Hart]

Add support for the MinnowBoard in the pch_gbe driver. This was
originally sent to LKML as part of the MinnowBoard support series. That
is now partially merged and this version of the patch has been isolated
from those changes and is now completely self-contained.

Thanks to everyone on Cc for their time in reviewing, the patch series
is better for it.

Since the previous version:
  Whitespace cleanup
  Remove trailing period in messages
  Comment formatting (added a colon)
  Use devm_gpio_request_one
  Fix missing adapter->pdata NULL check in PHY
  Use netdev style comment blocks
  Use kernel_ulong_t for driver_data cast
  Use unsigned for gpio and unsigned long for flags

The following changes since commit 0887a576a17965732270b2f8d37821fc02ef2feb:

  net/velocity: add poll controller function for velocity nic (2013-07-19 17:34:41 -0700)

are available in the git repository at:

  git://git.infradead.org/users/dvhart/linux-2.6.git minnow/net-next
  ssh://git.infradead.org/srv/git/users/dvhart/linux-2.6.git minnow/net-next

and browsable here:

  http://git.infradead.org/users/dvhart/linux-2.6.git/shortlog/refs/heads/minnow/net-next

Darren Hart (2):
  pch_gbe: Use PCH_GBE_PHY_REGS_LEN instead of 32
  pch_gbe: Add MinnowBoard support

 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    | 14 ++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   | 52 +++++++++++-
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c    | 96 ++++++++++++++++++++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h    |  2 +
 4 files changed, 163 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[PATCH net-next 1/2] pch_gbe: Use PCH_GBE_PHY_REGS_LEN instead of 32

[Darren Hart]

Avoid using magic numbers when we have perfectly good defines just lying
around.

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: netdev@vger.kernel.org
---
 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index ab1039a..749ddd9 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -682,7 +682,7 @@ static int pch_gbe_init_phy(struct pch_gbe_adapter *adapter)
 	}
 	adapter->hw.phy.addr = adapter->mii.phy_id;
 	netdev_dbg(netdev, "phy_addr = %d\n", adapter->mii.phy_id);
-	if (addr == 32)
+	if (addr == PCH_GBE_PHY_REGS_LEN)
 		return -EAGAIN;
 	/* Selected the phy and isolate the rest */
 	for (addr = 0; addr < PCH_GBE_PHY_REGS_LEN; addr++) {
-- 
1.8.3.1

[PATCH V2 net-next 2/2] pch_gbe: Add MinnowBoard support

[Darren Hart]

The MinnowBoard uses an AR803x PHY with the PCH GBE which requires
special handling. Use the MinnowBoard PCI Subsystem ID to detect this
and add a pci_device_id.driver_data structure and functions to handle
platform setup.

The AR803x does not implement the RGMII 2ns TX clock delay in the trace
routing nor via strapping. Add a detection method for the board and the
PHY and enable the TX clock delay via the registers.

This PHY will hibernate without link for 10 seconds. Ensure the PHY is
awake for probe and then disable hibernation. A future improvement would
be to convert pch_gbe to using PHYLIB and making sure we can wake the
PHY at the necessary times rather than permanently disabling it.

Signed-off-by: Darren Hart <dvhart@linux.intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Joe Perches <joe@perches.com>
Cc: netdev@vger.kernel.org
---
 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    | 14 ++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   | 50 +++++++++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c    | 96 ++++++++++++++++++++++
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h    |  2 +
 4 files changed, 162 insertions(+)

diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
index 7779036..c6a7269 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h
@@ -581,6 +581,18 @@ struct pch_gbe_hw_stats {
 	u32 intr_tcpip_err_count;
 };
 
+/* struct pch_gbe_privdata - PCI Device ID driver data
+ * @phy_tx_clk_delay:		Bool, configure the PHY TX delay in software
+ * @phy_disable_hibernate:	Bool, disable PHY hibernation
+ * @platform_init:		Platform initialization callback, called from
+ *				probe, prior to PHY initialization.
+ */
+struct pch_gbe_privdata {
+	bool phy_tx_clk_delay;
+	bool phy_disable_hibernate;
+	int(*platform_init)(struct pci_dev *pdev);
+};
+
 /**
  * struct pch_gbe_adapter - board specific private data structure
  * @stats_lock:	Spinlock structure for status
@@ -604,6 +616,7 @@ struct pch_gbe_hw_stats {
  * @rx_buffer_len:	Receive buffer length
  * @tx_queue_len:	Transmit queue length
  * @have_msi:		PCI MSI mode flag
+ * @pch_gbe_privdata:	PCI Device ID driver_data
  */
 
 struct pch_gbe_adapter {
@@ -631,6 +644,7 @@ struct pch_gbe_adapter {
 	int hwts_tx_en;
 	int hwts_rx_en;
 	struct pci_dev *ptp_pdev;
+	struct pch_gbe_privdata *pdata;
 };
 
 #define pch_gbe_hw_to_adapter(hw)	container_of(hw, struct pch_gbe_adapter, hw)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index 749ddd9..1f8840f 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -23,6 +23,8 @@
 #include <linux/module.h>
 #include <linux/net_tstamp.h>
 #include <linux/ptp_classify.h>
+#include <linux/gpio.h>
+#include <linux/mod_devicetable.h>
 
 #define DRV_VERSION     "1.01"
 const char pch_driver_version[] = DRV_VERSION;
@@ -111,6 +113,8 @@ const char pch_driver_version[] = DRV_VERSION;
 #define PTP_L4_MULTICAST_SA "01:00:5e:00:01:81"
 #define PTP_L2_MULTICAST_SA "01:1b:19:00:00:00"
 
+#define MINNOW_PHY_RESET_GPIO		13
+
 static unsigned int copybreak __read_mostly = PCH_GBE_COPYBREAK_DEFAULT;
 
 static int pch_gbe_mdio_read(struct net_device *netdev, int addr, int reg);
@@ -2635,6 +2639,9 @@ static int pch_gbe_probe(struct pci_dev *pdev,
 	adapter->pdev = pdev;
 	adapter->hw.back = adapter;
 	adapter->hw.reg = pcim_iomap_table(pdev)[PCH_GBE_PCI_BAR];
+	adapter->pdata = (struct pch_gbe_privdata *)pci_id->driver_data;
+	if (adapter->pdata && adapter->pdata->platform_init)
+		adapter->pdata->platform_init(pdev);
 
 	adapter->ptp_pdev = pci_get_bus_and_slot(adapter->pdev->bus->number,
 					       PCI_DEVFN(12, 4));
@@ -2710,6 +2717,10 @@ static int pch_gbe_probe(struct pci_dev *pdev,
 
 	dev_dbg(&pdev->dev, "PCH Network Connection\n");
 
+	/* Disable hibernation on certain platforms */
+	if (adapter->pdata && adapter->pdata->phy_disable_hibernate)
+		pch_gbe_phy_disable_hibernate(&adapter->hw);
+
 	device_set_wakeup_enable(&pdev->dev, 1);
 	return 0;
 
@@ -2720,9 +2731,48 @@ err_free_netdev:
 	return ret;
 }
 
+/* The AR803X PHY on the MinnowBoard requires a physical pin to be toggled to
+ * ensure it is awake for probe and init. Request the line and reset the PHY.
+ */
+static int pch_gbe_minnow_platform_init(struct pci_dev *pdev)
+{
+	unsigned long flags = GPIOF_DIR_OUT | GPIOF_INIT_HIGH | GPIOF_EXPORT;
+	unsigned gpio = MINNOW_PHY_RESET_GPIO;
+	int ret;
+
+	ret = devm_gpio_request_one(&pdev->dev, gpio, flags,
+				    "minnow_phy_reset");
+	if (ret) {
+		dev_err(&pdev->dev,
+			"ERR: Can't request PHY reset GPIO line '%d'\n", gpio);
+		return ret;
+	}
+
+	gpio_set_value(gpio, 0);
+	usleep_range(1250, 1500);
+	gpio_set_value(gpio, 1);
+	usleep_range(1250, 1500);
+
+	return ret;
+}
+
+static struct pch_gbe_privdata pch_gbe_minnow_privdata = {
+	.phy_tx_clk_delay = true,
+	.phy_disable_hibernate = true,
+	.platform_init = pch_gbe_minnow_platform_init,
+};
+
 static DEFINE_PCI_DEVICE_TABLE(pch_gbe_pcidev_id) = {
 	{.vendor = PCI_VENDOR_ID_INTEL,
 	 .device = PCI_DEVICE_ID_INTEL_IOH1_GBE,
+	 .subvendor = PCI_VENDOR_ID_CIRCUITCO,
+	 .subdevice = PCI_SUBSYSTEM_ID_CIRCUITCO_MINNOWBOARD,
+	 .class = (PCI_CLASS_NETWORK_ETHERNET << 8),
+	 .class_mask = (0xFFFF00),
+	 .driver_data = (kernel_ulong_t)&pch_gbe_minnow_privdata
+	 },
+	{.vendor = PCI_VENDOR_ID_INTEL,
+	 .device = PCI_DEVICE_ID_INTEL_IOH1_GBE,
 	 .subvendor = PCI_ANY_ID,
 	 .subdevice = PCI_ANY_ID,
 	 .class = (PCI_CLASS_NETWORK_ETHERNET << 8),
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
index da07907..be513a9 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
@@ -74,6 +74,15 @@
 #define MII_SR_100X_FD_CAPS      0x4000	/* 100X  Full Duplex Capable */
 #define MII_SR_100T4_CAPS        0x8000	/* 100T4 Capable */
 
+/* AR8031 PHY Debug Registers */
+#define PHY_AR803X_ID           0x00001374
+#define PHY_AR8031_DBG_OFF      0x1D
+#define PHY_AR8031_DBG_DAT      0x1E
+#define PHY_AR8031_SERDES       0x05
+#define PHY_AR8031_HIBERNATE    0x0B
+#define PHY_AR8031_SERDES_TX_CLK_DLY   0x0100 /* TX clock delay of 2.0ns */
+#define PHY_AR8031_PS_HIB_EN           0x8000 /* Hibernate enable */
+
 /* Phy Id Register (word 2) */
 #define PHY_REVISION_MASK        0x000F
 
@@ -277,4 +286,91 @@ void pch_gbe_phy_init_setting(struct pch_gbe_hw *hw)
 	pch_gbe_phy_read_reg_miic(hw, PHY_PHYSP_CONTROL, &mii_reg);
 	mii_reg |= PHYSP_CTRL_ASSERT_CRS_TX;
 	pch_gbe_phy_write_reg_miic(hw, PHY_PHYSP_CONTROL, mii_reg);
+
+	/* Setup a TX clock delay on certain platforms */
+	if (adapter->pdata && adapter->pdata->phy_tx_clk_delay)
+		pch_gbe_phy_tx_clk_delay(hw);
+}
+
+/* pch_gbe_phy_tx_clk_delay - Setup TX clock delay via the PHY
+ * @hw:	            Pointer to the HW structure
+ * Returns
+ *	0:		Successful.
+ *	-EINVAL:	Invalid argument.
+ */
+int pch_gbe_phy_tx_clk_delay(struct pch_gbe_hw *hw)
+{
+	/* The RGMII interface requires a ~2ns TX clock delay. This is typically
+	 * done in layout with a longer trace or via PHY strapping, but can also
+	 * be done via PHY configuration registers.
+	 */
+	struct pch_gbe_adapter *adapter = pch_gbe_hw_to_adapter(hw);
+	u16 mii_reg;
+	int ret = 0;
+
+	switch (hw->phy.id) {
+	case PHY_AR803X_ID:
+		netdev_dbg(adapter->netdev,
+			   "Configuring AR803X PHY for 2ns TX clock delay\n");
+		pch_gbe_phy_read_reg_miic(hw, PHY_AR8031_DBG_OFF, &mii_reg);
+		ret = pch_gbe_phy_write_reg_miic(hw, PHY_AR8031_DBG_OFF,
+						 PHY_AR8031_SERDES);
+		if (ret)
+			break;
+
+		pch_gbe_phy_read_reg_miic(hw, PHY_AR8031_DBG_DAT, &mii_reg);
+		mii_reg |= PHY_AR8031_SERDES_TX_CLK_DLY;
+		ret = pch_gbe_phy_write_reg_miic(hw, PHY_AR8031_DBG_DAT,
+						 mii_reg);
+		break;
+	default:
+		netdev_err(adapter->netdev,
+			   "Unknown PHY (%x), could not set TX clock delay\n",
+			   hw->phy.id);
+		return -EINVAL;
+	}
+
+	if (ret)
+		netdev_err(adapter->netdev,
+			   "Could not configure tx clock delay for PHY\n");
+	return ret;
+}
+
+/* pch_gbe_phy_disable_hibernate - Disable the PHY low power state
+ * @hw:	            Pointer to the HW structure
+ * Returns
+ *	0:		Successful.
+ *	-EINVAL:	Invalid argument.
+ */
+int pch_gbe_phy_disable_hibernate(struct pch_gbe_hw *hw)
+{
+	struct pch_gbe_adapter *adapter = pch_gbe_hw_to_adapter(hw);
+	u16 mii_reg;
+	int ret = 0;
+
+	switch (hw->phy.id) {
+	case PHY_AR803X_ID:
+		netdev_dbg(adapter->netdev,
+			   "Disabling hibernation for AR803X PHY\n");
+		ret = pch_gbe_phy_write_reg_miic(hw, PHY_AR8031_DBG_OFF,
+						 PHY_AR8031_HIBERNATE);
+		if (ret)
+			break;
+
+		pch_gbe_phy_read_reg_miic(hw, PHY_AR8031_DBG_DAT, &mii_reg);
+		mii_reg &= ~PHY_AR8031_PS_HIB_EN;
+		ret = pch_gbe_phy_write_reg_miic(hw, PHY_AR8031_DBG_DAT,
+						 mii_reg);
+		break;
+	default:
+		netdev_err(adapter->netdev,
+			   "Unknown PHY (%x), could not disable hibernation\n",
+			   hw->phy.id);
+		return -EINVAL;
+	}
+
+	if (ret)
+		netdev_err(adapter->netdev,
+			   "Could not disable PHY hibernation\n");
+	return ret;
 }
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
index 03264dc..e3e4bc9 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
@@ -33,5 +33,7 @@ void pch_gbe_phy_power_up(struct pch_gbe_hw *hw);
 void pch_gbe_phy_power_down(struct pch_gbe_hw *hw);
 void pch_gbe_phy_set_rgmii(struct pch_gbe_hw *hw);
 void pch_gbe_phy_init_setting(struct pch_gbe_hw *hw);
+int pch_gbe_phy_tx_clk_delay(struct pch_gbe_hw *hw);
+int pch_gbe_phy_disable_hibernate(struct pch_gbe_hw *hw);
 
 #endif /* _PCH_GBE_PHY_H_ */
-- 
1.8.3.1

Re: [PATCH V2 net-next 2/2] pch_gbe: Add MinnowBoard support

[David Miller]

From: Darren Hart <dvhart@linux.intel.com>
Date: Mon, 22 Jul 2013 14:41:59 -0700

> diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
> index da07907..be513a9 100644
> --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
> +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
 ...
> @@ -277,4 +286,91 @@ void pch_gbe_phy_init_setting(struct pch_gbe_hw *hw)
>  	pch_gbe_phy_read_reg_miic(hw, PHY_PHYSP_CONTROL, &mii_reg);
>  	mii_reg |= PHYSP_CTRL_ASSERT_CRS_TX;
>  	pch_gbe_phy_write_reg_miic(hw, PHY_PHYSP_CONTROL, mii_reg);
> +
> +	/* Setup a TX clock delay on certain platforms */
> +	if (adapter->pdata && adapter->pdata->phy_tx_clk_delay)
> +		pch_gbe_phy_tx_clk_delay(hw);
> +}
> +
> +/* pch_gbe_phy_tx_clk_delay - Setup TX clock delay via the PHY
> + * @hw:	            Pointer to the HW structure
> + * Returns
> + *	0:		Successful.
> + *	-EINVAL:	Invalid argument.
> + */
> +int pch_gbe_phy_tx_clk_delay(struct pch_gbe_hw *hw)
 ...
> diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
> index 03264dc..e3e4bc9 100644
> --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
> +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
> @@ -33,5 +33,7 @@ void pch_gbe_phy_power_up(struct pch_gbe_hw *hw);
>  void pch_gbe_phy_power_down(struct pch_gbe_hw *hw);
>  void pch_gbe_phy_set_rgmii(struct pch_gbe_hw *hw);
>  void pch_gbe_phy_init_setting(struct pch_gbe_hw *hw);
> +int pch_gbe_phy_tx_clk_delay(struct pch_gbe_hw *hw);
> +int pch_gbe_phy_disable_hibernate(struct pch_gbe_hw *hw);


pch_gbe_phy_tx_clk_delay() is only invoked from pch_gbe_phy.c, where it is also
implemented.

Therefore it should be marked static and the declaration removed from
pch_gbe_phy.h

Thanks.

Re: [PATCH V2 net-next 2/2] pch_gbe: Add MinnowBoard support

[Darren Hart]

On Wed, 2013-07-24 at 17:00 -0700, David Miller wrote:
> From: Darren Hart <dvhart@linux.intel.com>
> Date: Mon, 22 Jul 2013 14:41:59 -0700
> 
> > diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
> > index da07907..be513a9 100644
> > --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
> > +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
>  ...
> > @@ -277,4 +286,91 @@ void pch_gbe_phy_init_setting(struct pch_gbe_hw *hw)
> >       pch_gbe_phy_read_reg_miic(hw, PHY_PHYSP_CONTROL, &mii_reg);
> >       mii_reg |= PHYSP_CTRL_ASSERT_CRS_TX;
> >       pch_gbe_phy_write_reg_miic(hw, PHY_PHYSP_CONTROL, mii_reg);
> > +
> > +     /* Setup a TX clock delay on certain platforms */
> > +     if (adapter->pdata && adapter->pdata->phy_tx_clk_delay)
> > +             pch_gbe_phy_tx_clk_delay(hw);
> > +}
> > +
> > +/* pch_gbe_phy_tx_clk_delay - Setup TX clock delay via the PHY
> > + * @hw:                  Pointer to the HW structure
> > + * Returns
> > + *   0:              Successful.
> > + *   -EINVAL:        Invalid argument.
> > + */
> > +int pch_gbe_phy_tx_clk_delay(struct pch_gbe_hw *hw)
>  ...
> > diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
> > index 03264dc..e3e4bc9 100644
> > --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
> > +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.h
> > @@ -33,5 +33,7 @@ void pch_gbe_phy_power_up(struct pch_gbe_hw *hw);
> >  void pch_gbe_phy_power_down(struct pch_gbe_hw *hw);
> >  void pch_gbe_phy_set_rgmii(struct pch_gbe_hw *hw);
> >  void pch_gbe_phy_init_setting(struct pch_gbe_hw *hw);
> > +int pch_gbe_phy_tx_clk_delay(struct pch_gbe_hw *hw);
> > +int pch_gbe_phy_disable_hibernate(struct pch_gbe_hw *hw);
> 
> 
> pch_gbe_phy_tx_clk_delay() is only invoked from pch_gbe_phy.c, where it is also
> implemented.
> 
> Therefore it should be marked static and the declaration removed from
> pch_gbe_phy.h

Ah, thanks. Leftover from a more complex driver_data with pointers to
these functions. I'll fix and resubmit this patch alone as V3.
Otherwise, no objections?

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [PATCH V2 net-next 2/2] pch_gbe: Add MinnowBoard support

[David Miller]

From: Darren Hart <dvhart@linux.intel.com>
Date: Wed, 24 Jul 2013 17:20:03 -0700

> Otherwise, no objections?

None from me.

Re: [PATCH V2 net-next 2/2] pch_gbe: Add MinnowBoard support

[Joe Perches]

On Wed, 2013-07-24 at 17:20 -0700, Darren Hart wrote:
> Ah, thanks. Leftover from a more complex driver_data with pointers to
> these functions. I'll fix and resubmit this patch alone as V3.
> Otherwise, no objections?

Only small nit is not using kernel-doc for this bit below:

On Mon, 2013-07-22 at 14:41 -0700, Darren Hart wrote:
> diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
> []
> +/* pch_gbe_phy_tx_clk_delay - Setup TX clock delay via the PHY
> + * @hw:	            Pointer to the HW structure
> + * Returns
> + *	0:		Successful.
> + *	-EINVAL:	Invalid argument.
> + */

Re: [PATCH V2 net-next 2/2] pch_gbe: Add MinnowBoard support

[Darren Hart]

On Wed, 2013-07-24 at 17:41 -0700, Joe Perches wrote:
> On Wed, 2013-07-24 at 17:20 -0700, Darren Hart wrote:
> > Ah, thanks. Leftover from a more complex driver_data with pointers to
> > these functions. I'll fix and resubmit this patch alone as V3.
> > Otherwise, no objections?
> 
> Only small nit is not using kernel-doc for this bit below:

Hi Joe,

Thank you for the review.

I was following the existing style of the file (with the exception of
the leading two ** as checkpatch complains. I thought erring on the side
of consistency was the best course of action here. What would you
suggest is the preferred approach here?

> 
> On Mon, 2013-07-22 at 14:41 -0700, Darren Hart wrote:
> > diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
> > []
> > +/* pch_gbe_phy_tx_clk_delay - Setup TX clock delay via the PHY
> > + * @hw:	            Pointer to the HW structure
> > + * Returns
> > + *	0:		Successful.
> > + *	-EINVAL:	Invalid argument.
> > + */
> 
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [PATCH V2 net-next 2/2] pch_gbe: Add MinnowBoard support

[Joe Perches]

On Wed, 2013-07-24 at 19:58 -0700, Darren Hart wrote:
> On Wed, 2013-07-24 at 17:41 -0700, Joe Perches wrote:
> > On Wed, 2013-07-24 at 17:20 -0700, Darren Hart wrote:
> > > Ah, thanks. Leftover from a more complex driver_data with pointers to
> > > these functions. I'll fix and resubmit this patch alone as V3.
> > > Otherwise, no objections?
> > 
> > Only small nit is not using kernel-doc for this bit below:
[]
> I was following the existing style of the file (with the exception of
> the leading two ** as checkpatch complains. I thought erring on the side
> of consistency was the best course of action here. What would you
> suggest is the preferred approach here?

Hi Darren.

I hadn't actually looked at the rest of the unpatched
file.  If what you did is the same style, it's fine.

It just looked like you were trying to use kernel-doc
formatting style but not using it quite correctly.

> > On Mon, 2013-07-22 at 14:41 -0700, Darren Hart wrote:
> > > diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
> > > []
> > > +/* pch_gbe_phy_tx_clk_delay - Setup TX clock delay via the PHY
> > > + * @hw:	            Pointer to the HW structure
> > > + * Returns
> > > + *	0:		Successful.
> > > + *	-EINVAL:	Invalid argument.
> > > + */

This bit in what I believe kernel-doc style is would
look like:

/**
 * pch_gbe_phy_tx_clk_delay - Setup TX clock delay via the PHY
 * @hw:		Pointer to the HW structure
 *
 * Return:	O: Successful
 *		-EINVAL: Invalid argument
 */

Re: [PATCH V2 net-next 2/2] pch_gbe: Add MinnowBoard support

[Darren Hart]

On Wed, 2013-07-24 at 20:08 -0700, Joe Perches wrote:
> On Wed, 2013-07-24 at 19:58 -0700, Darren Hart wrote:
> > On Wed, 2013-07-24 at 17:41 -0700, Joe Perches wrote:
> > > On Wed, 2013-07-24 at 17:20 -0700, Darren Hart wrote:
> > > > Ah, thanks. Leftover from a more complex driver_data with pointers to
> > > > these functions. I'll fix and resubmit this patch alone as V3.
> > > > Otherwise, no objections?
> > > 
> > > Only small nit is not using kernel-doc for this bit below:
> []
> > I was following the existing style of the file (with the exception of
> > the leading two ** as checkpatch complains. I thought erring on the side
> > of consistency was the best course of action here. What would you
> > suggest is the preferred approach here?
> 
> Hi Darren.
> 
> I hadn't actually looked at the rest of the unpatched
> file.  If what you did is the same style, it's fine.
> 
> It just looked like you were trying to use kernel-doc
> formatting style but not using it quite correctly.
> 
> > > On Mon, 2013-07-22 at 14:41 -0700, Darren Hart wrote:
> > > > diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_phy.c
> > > > []
> > > > +/* pch_gbe_phy_tx_clk_delay - Setup TX clock delay via the PHY
> > > > + * @hw:	            Pointer to the HW structure
> > > > + * Returns
> > > > + *	0:		Successful.
> > > > + *	-EINVAL:	Invalid argument.
> > > > + */
> 
> This bit in what I believe kernel-doc style is would
> look like:
> 
> /**
>  * pch_gbe_phy_tx_clk_delay - Setup TX clock delay via the PHY
>  * @hw:		Pointer to the HW structure
>  *
>  * Return:	O: Successful
>  *		-EINVAL: Invalid argument
>  */

Right, but /**\n is discouraged? outlawed? on netdev. I'm not sure what
the kernel-doc policy would be for netdev... I think a follow-up patch
is in order for this driver that continues to fix a variety of
formatting, coding style, and api usage violations in addition to those
Andy already provided.

I'll resubmit with the fix David asked for to get this functionality in
and consider a clean-up as a follow-on.

Thanks again!

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

Re: [PATCH V2 net-next 2/2] pch_gbe: Add MinnowBoard support

[Joe Perches]

On Wed, 2013-07-24 at 19:58 -0700, Darren Hart wrote:
> I was following the existing style of the file (with the exception of
> the leading two ** as checkpatch complains.

I just tested and duplicated that.

It's a checkpatch defect.
I'll fix it.  Ignore it for now.

cheers, Joe

[PATCH] checkpatch: Fix networking kernel-doc block comment defect

[Joe Perches]

checkpatch can generate a false positive when inserting
a new kernel-doc block and function above an existing
kernel-doc block.

Fix it by checking that the context line is also a newly
inserted line.

Reported-by: Darren Hart <dvhart@linux.intel.com>
Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 23126d4..c18e1be 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2074,6 +2074,7 @@ sub process {
 		if ($realfile =~ m@^(drivers/net/|net/)@ &&
 		    $prevrawline =~ /^\+[ \t]*\/\*/ &&		#starting /*
 		    $prevrawline !~ /\*\/[ \t]*$/ &&		#no trailing */
+		    $rawline =~ /^\+/ &&			#line is new
 		    $rawline !~ /^\+[ \t]*\*/) {		#no leading *
 			WARN("NETWORKING_BLOCK_COMMENT_STYLE",
 			     "networking block comments start with * on subsequent lines\n" . $hereprev);
-- 
1.8.1.2.459.gbcd45b4.dirty