Re: [PATCH net-next v2 01/16] net: txgbe: Store PCI info

Please avoid macro magic, unless really needed.

+
+/* struct txgbe_mac_operations */
+static void txgbe_bus_set_lan_id_dummy(struct txgbe_hw *TUP0)
+{
+}

Seems pointless, so just remove this function, and then you don't need
the macro magic.

+
+static void txgbe_init_ops_dummy(struct txgbe_hw *hw)
+{
+	struct txgbe_mac_info *mac = &hw->mac;
+
+	/* MAC */
+	mac->ops.set_lan_id = txgbe_bus_set_lan_id_dummy;

Just set it to NULL. A user can first check if its NULL, and not call
it, or return -EOPNOTSUPP etc.

+/* cmd_addr is used for some special command:
+ * 1. to be sector address, when implemented erase sector command
+ * 2. to be flash address when implemented read, write flash address
+ */
+int txgbe_fmgr_cmd_op(struct txgbe_hw *hw, u32 cmd, u32 cmd_addr)
+{
+	u32 cmd_val = 0, val = 0;
+
+	cmd_val = (cmd << SPI_CLK_CMD_OFFSET) |
+		  (SPI_CLK_DIV << SPI_CLK_DIV_OFFSET) | cmd_addr;
+	wr32(hw, SPI_H_CMD_REG_ADDR, cmd_val);
+
+	return read_poll_timeout(rd32, val, (val & 0x1), 10, SPI_TIME_OUT_VALUE,
+				 false, hw, SPI_H_STA_REG_ADDR);
+}
+

+int txgbe_flash_read_dword(struct txgbe_hw *hw, u32 addr, u32 *data)
+{
+	int ret = 0;
+
+	ret = txgbe_fmgr_cmd_op(hw, SPI_CMD_READ_DWORD, addr);
+	if (ret == -ETIMEDOUT)
+		return ret;

Are you absolutely sure it will never return any other error code?
The pattern in the kernel is

+	if (ret)
+		return ret;

or

+	if (ret < 0)
+		return ret;

+int txgbe_check_flash_load(struct txgbe_hw *hw, u32 check_bit)
+{
+	u32 i = 0, reg = 0;
+	int err = 0;
+
+	/* if there's flash existing */
+	if (!(rd32(hw, TXGBE_SPI_STATUS) &
+	      TXGBE_SPI_STATUS_FLASH_BYPASS)) {
+		/* wait hw load flash done */
+		for (i = 0; i < TXGBE_MAX_FLASH_LOAD_POLL_TIME; i++) {
+			reg = rd32(hw, TXGBE_SPI_ILDR_STATUS);
+			if (!(reg & check_bit)) {
+				/* done */
+				break;
+			}
+			msleep(200);
+		}

This is what iopoll.h is for. Any sort of loop waiting for something
to happen should use one of the helpers in there.

+		if (i == TXGBE_MAX_FLASH_LOAD_POLL_TIME) {
+			err = -ETIMEDOUT;
+			txgbe_info(hw, "Check flash load timeout.\n");

Either dev_err() if this is fatal, or dev_dbg() if it is
not. dev_info() is not used very often, maybe just in your probe
function when you have found your hardware.

+static int txgbe_sw_init(struct txgbe_adapter *adapter)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct txgbe_hw *hw = &adapter->hw;
+	u32 ssid = 0;
+	int err = 0;
+
+	/* PCI config space info */
+	hw->vendor_id = pdev->vendor;
+	hw->device_id = pdev->device;
+	hw->revision_id = pdev->revision;
+	hw->oem_svid = pdev->subsystem_vendor;
+	hw->oem_ssid = pdev->subsystem_device;
+
+	if (hw->oem_svid == PCI_VENDOR_ID_WANGXUN) {
+		hw->subsystem_vendor_id = pdev->subsystem_vendor;
+		hw->subsystem_device_id = pdev->subsystem_device;
+	} else {
+		err = txgbe_flash_read_dword(hw, 0xfffdc, &ssid);
+		if (err == -ETIMEDOUT) {
+			netif_err(adapter, probe, adapter->netdev,
+				  "read of internal subsystem device id failed\n");
+			return -ENODEV;

Don't turn an -ETIMEDOUT into an -ENODEV.  In general, never convert
from one error code to another, unless you have a really good reason,
and if you do, comment why. -ETIMEDOUT is useful information, it helps
narrow down the problem.

These should be in the .c file. Avoid them in header files, since you
will include more headers than actually needed, slowing down the
build. A lot of time is spent in the build processing useless
headers. Please don't make it worse.

Generally that would be BIT(0), BIT(1), BIT(2), BIT(29), BIT(30),
BIT(31).  I find this easier to read, especially when it is bit(21),
somewhere in the middle.

Please consider changing all the #defines that are for bits.

       Andrew