forcedeth: Addition of new device id (resend without pci_ids.h)

PATCH In review Ayaz Abdulla 2 participants 5 messages Jun 3, 2009

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Ayaz Abdulla

author

2009-06-03 21:38 UTC #0 lore raw

PATCH 1/1 forcedeth: Addition of new device id (resend without pci_ids.h)

Add support for new ethernet device in the MCP89 chipset.

Signed-off-by Ayaz Abdulla <[email protected]>

drivers/net/forcedeth.c +11 -1

@@ -70,6 +70,11 @@
 #define dprintk(x...)		do { } while (0)
 #endif
 
+/*
+ * Hardware device ids (no longer updating pci_ids.h)
+ */
+#define PCI_DEVICE_ID_NVIDIA_NVENET_40  0x0D7D
+
 #define TX_WORK_PER_LOOP  64
 #define RX_WORK_PER_LOOP  64

@@ -6149,7 +6154,8 @@
 	for (i = 0;i <= np->register_size/sizeof(u32); i++)
 		writel(np->saved_config_space[i], base+i*sizeof(u32));
 
-	pci_write_config_dword(pdev, NV_MSI_PRIV_OFFSET, NV_MSI_PRIV_VALUE);
+	if (np->device_id != PCI_DEVICE_ID_NVIDIA_NVENET_40)
+		pci_write_config_dword(pdev, NV_MSI_PRIV_OFFSET, NV_MSI_PRIV_VALUE);
 
 	/* restore phy state, including autoneg */
 	phy_init(dev);

@@ -6353,6 +6359,10 @@
 		PCI_DEVICE(PCI_VENDOR_ID_NVIDIA, PCI_DEVICE_ID_NVIDIA_NVENET_39),
 		.driver_data = DEV_NEED_LINKTIMER|DEV_HAS_LARGEDESC|DEV_HAS_CHECKSUM|DEV_HAS_HIGH_DMA|DEV_HAS_MSI|DEV_HAS_POWER_CNTRL|DEV_HAS_PAUSEFRAME_TX_V3|DEV_HAS_STATISTICS_V3|DEV_HAS_TEST_EXTENDED|DEV_HAS_CORRECT_MACADDR|DEV_HAS_COLLISION_FIX|DEV_NEED_TX_LIMIT|DEV_HAS_GEAR_MODE,
 	},
+	{	/* MCP89 Ethernet Controller */
+		PCI_DEVICE(PCI_VENDOR_ID_NVIDIA, PCI_DEVICE_ID_NVIDIA_NVENET_40),
+		.driver_data = DEV_NEED_LINKTIMER|DEV_HAS_LARGEDESC|DEV_HAS_CHECKSUM|DEV_HAS_HIGH_DMA|DEV_HAS_MSI|DEV_HAS_POWER_CNTRL|DEV_HAS_PAUSEFRAME_TX_V3|DEV_HAS_STATISTICS_V3|DEV_HAS_TEST_EXTENDED|DEV_HAS_CORRECT_MACADDR|DEV_HAS_COLLISION_FIX|DEV_NEED_TX_LIMIT|DEV_HAS_GEAR_MODE,
+	},
 	{0,},
 };

David Miller

maintainer

2009-06-03 21:44 UTC #1 lore raw

From: Ayaz Abdulla <[email protected]>
Date: Wed, 03 Jun 2009 12:06:04 -0400

Quoted 3 lines

Add support for new ethernet device in the MCP89 chipset.
Signed-off-by Ayaz Abdulla <[email protected]>

Just use the constant directly, please :-)

The new macro is completely superfluous.

Trust me, other drivers just use the PCI ID constant directly in their tables too, and no animals were harmed by the experiment :-)

Ayaz Abdulla

author

2009-06-03 21:57 UTC #2 lore raw

Quoted 10 lines · David Miller

From: Ayaz Abdulla <[email protected]>
Date: Wed, 03 Jun 2009 12:06:04 -0400
Quoted 3 lines
Add support for new ethernet device in the MCP89 chipset.
Signed-off-by Ayaz Abdulla <[email protected]>
Just use the constant directly, please :-)

But there is place in the patch that refers to device id so it makes more sense to have a macro. i.e

+ if (np->device_id != PCI_DEVICE_ID_NVIDIA_NVENET_40)

other places in the code also refer to device ids for certain workarounds. i.e.

		if ((id->device == PCI_DEVICE_ID_NVIDIA_NVENET_12 ||
		     id->device == PCI_DEVICE_ID_NVIDIA_NVENET_13) &&

Quoted 5 lines

The new macro is completely superfluous.
Trust me, other drivers just use the PCI ID constant directly in their tables too, and no animals were harmed by the experiment :-)

David Miller

maintainer

2009-06-03 21:59 UTC #3 lore raw

From: Ayaz Abdulla <[email protected]>
Date: Wed, 03 Jun 2009 12:24:37 -0400

Quoted 14 lines

Quoted 7 lines · David Miller
From: Ayaz Abdulla <[email protected]>
Date: Wed, 03 Jun 2009 12:06:04 -0400
Quoted 3 lines
Add support for new ethernet device in the MCP89 chipset.
Signed-off-by Ayaz Abdulla <[email protected]>
Just use the constant directly, please :-)
But there is place in the patch that refers to device id so it makes more sense to have a macro. i.e

Quoted 7 lines

+ if (np->device_id != PCI_DEVICE_ID_NVIDIA_NVENET_40)
other places in the code also refer to device ids for certain workarounds. i.e.
		if ((id->device == PCI_DEVICE_ID_NVIDIA_NVENET_12 ||
		     id->device == PCI_DEVICE_ID_NVIDIA_NVENET_13) &&

And you can add the define when that is necessary.

Otherwise, it's being used in exactly one location, the ID table. And for that purpose it's superfluous.

You could also change the driver to use a bitmask of HW workaround cases, that gets set very early in the probe. And the HW workarounds are keyed on those bit masks (one test) compared to the direct ID checks (potentially multiple compares).

It would be both cleaner and more efficient. And you could still use the direct constants without any real loss of clarity.

Ayaz Abdulla

author

2009-06-03 22:16 UTC #4 lore raw

Quoted 34 lines · David Miller

From: Ayaz Abdulla <[email protected]>
Date: Wed, 03 Jun 2009 12:24:37 -0400
Quoted 16 lines
Quoted 9 lines · David Miller
From: Ayaz Abdulla <[email protected]>
Date: Wed, 03 Jun 2009 12:06:04 -0400
Quoted 3 lines
Add support for new ethernet device in the MCP89 chipset.
Signed-off-by Ayaz Abdulla <[email protected]>
Just use the constant directly, please :-)
But there is place in the patch that refers to device id so it makes more sense to have a macro. i.e
Quoted 7 lines
+ if (np->device_id != PCI_DEVICE_ID_NVIDIA_NVENET_40)
other places in the code also refer to device ids for certain workarounds. i.e.
		if ((id->device == PCI_DEVICE_ID_NVIDIA_NVENET_12 ||
		     id->device == PCI_DEVICE_ID_NVIDIA_NVENET_13) &&
And you can add the define when that is necessary.
Otherwise, it's being used in exactly one location, the ID table. And for that purpose it's superfluous.

Quoted 7 lines

You could also change the driver to use a bitmask of HW workaround cases, that gets set very early in the probe. And the HW workarounds are keyed on those bit masks (one test) compared to the direct ID checks (potentially multiple compares).
It would be both cleaner and more efficient. And you could still use the direct constants without any real loss of clarity.

Ok, I will do that in next patch...