On 10/18/2018 20:14, Jakub Kicinski wrote:
On Wed, 17 Oct 2018 15:23:12 -0700, Jeff Kirsher wrote:
From: Sasha Neftin <sasha.nef...@intel.com>
This patch adds the beginning framework onto which I am going to add
the igc driver which supports the Intel(R) I225-LM/I225-V 2.5G
Ethernet Controller.
Signed-off-by: Sasha Neftin <sasha.nef...@intel.com>
Tested-by: Aaron Brown <aaron.f.br...@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirs...@intel.com>
bunch of minor nit picks
diff --git a/drivers/net/ethernet/intel/igc/igc.h
b/drivers/net/ethernet/intel/igc/igc.h
new file mode 100644
index 000000000000..afe595cfcf63
--- /dev/null
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2018 Intel Corporation */
+
+#ifndef _IGC_H_
+#define _IGC_H_
+
+#include <linux/kobject.h>
+
+#include <linux/pci.h>
+#include <linux/netdevice.h>
+#include <linux/vmalloc.h>
+
+#include <linux/ethtool.h>
+
+#include <linux/sctp.h>
+
+#define IGC_ERR(args...) pr_err("igc: " args)
Looks like you're reimplementing pr_fmt()
yes, it is convenient for a debug. Same methodological approach I saw in
other drivers.
+#define PFX "igc: "
+
+#include <linux/timecounter.h>
+#include <linux/net_tstamp.h>
+#include <linux/ptp_clock_kernel.h>
Splitting the includes looks fairly weird. Also, you're not using any
of them here.
Good catch. I'll remove splits and unused "include"'s. All "include"'s
will be add per demand. I will send patch to fix that.
+/* main */
+extern char igc_driver_name[];
+extern char igc_driver_version[];
+
+#endif /* _IGC_H_ */
diff --git a/drivers/net/ethernet/intel/igc/igc_hw.h
b/drivers/net/ethernet/intel/igc/igc_hw.h
new file mode 100644
index 000000000000..aa68b4516700
--- /dev/null
+++ b/drivers/net/ethernet/intel/igc/igc_hw.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2018 Intel Corporation */
+
+#ifndef _IGC_HW_H_
+#define _IGC_HW_H_
+
+#define IGC_DEV_ID_I225_LM 0x15F2
+#define IGC_DEV_ID_I225_V 0x15F3
+
+#endif /* _IGC_HW_H_ */
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
b/drivers/net/ethernet/intel/igc/igc_main.c
new file mode 100644
index 000000000000..753749ce5ae0
--- /dev/null
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018 Intel Corporation */
+
+#include <linux/module.h>
+#include <linux/types.h>
+
+#include "igc.h"
+#include "igc_hw.h"
+
+#define DRV_VERSION "0.0.1-k"
You can just use the kernel version, it works pretty well in presence
of backports.
Good idea, I think I will do it too.
+#define DRV_SUMMARY "Intel(R) 2.5G Ethernet Linux Driver"
+
+MODULE_AUTHOR("Intel Corporation, <linux.n...@intel.com>");
+MODULE_DESCRIPTION(DRV_SUMMARY);
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION(DRV_VERSION);
+
+char igc_driver_name[] = "igc";
+char igc_driver_version[] = DRV_VERSION;
+static const char igc_driver_string[] = DRV_SUMMARY;
+static const char igc_copyright[] =
+ "Copyright(c) 2018 Intel Corporation.";
+
+static const struct pci_device_id igc_pci_tbl[] = {
+ { PCI_VDEVICE(INTEL, IGC_DEV_ID_I225_LM) },
+ { PCI_VDEVICE(INTEL, IGC_DEV_ID_I225_V) },
+ /* required last entry */
+ {0, }
+};
+
+MODULE_DEVICE_TABLE(pci, igc_pci_tbl);
+
+/**
+ * igc_probe - Device Initialization Routine
+ * @pdev: PCI device information struct
+ * @ent: entry in igc_pci_tbl
+ *
+ * Returns 0 on success, negative on failure
+ *
+ * igc_probe initializes an adapter identified by a pci_dev structure.
+ * The OS initialization, configuring the adapter private structure,
+ * and a hardware reset occur.
+ */
+static int igc_probe(struct pci_dev *pdev,
+ const struct pci_device_id *ent)
+{
+ int err, pci_using_dac;
+
+ err = pci_enable_device_mem(pdev);
+ if (err)
+ return err;
+
+ pci_using_dac = 0;
+ err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
+ if (!err) {
+ err = dma_set_coherent_mask(&pdev->dev,
+ DMA_BIT_MASK(64));
+ if (!err)
+ pci_using_dac = 1;
You never use this pci_using_dac. dma_set_mask_and_coherent() maybe?
Right. I saw already somebody sent out patch to fix that.
+ } else {
+ err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+ if (err) {
+ err = dma_set_coherent_mask(&pdev->dev,
+ DMA_BIT_MASK(32));
+ if (err) {
+ IGC_ERR("Wrong DMA configuration, aborting\n");
+ goto err_dma;
+ }
+ }
+ }
+
+ err = pci_request_selected_regions(pdev,
+ pci_select_bars(pdev,
+ IORESOURCE_MEM),
+ igc_driver_name);
+ if (err)
+ goto err_pci_reg;
+
+ pci_set_master(pdev);
+ err = pci_save_state(pdev);
And you never look at err?
Same as above.
+ return 0;
+
+err_pci_reg:
+err_dma:
The error label should be named after what it points to, not where it's
coming from.
+ pci_disable_device(pdev);
+ return err;
+}
+
+/**
+ * igc_remove - Device Removal Routine
+ * @pdev: PCI device information struct
+ *
+ * igc_remove is called by the PCI subsystem to alert the driver
+ * that it should release a PCI device. This could be caused by a
+ * Hot-Plug event, or because the driver is going to be removed from
+ * memory.
+ */
+static void igc_remove(struct pci_dev *pdev)
+{
+ pci_release_selected_regions(pdev,
+ pci_select_bars(pdev, IORESOURCE_MEM));
+
+ pci_disable_device(pdev);
+}
+
+static struct pci_driver igc_driver = {
+ .name = igc_driver_name,
+ .id_table = igc_pci_tbl,
+ .probe = igc_probe,
+ .remove = igc_remove,
+};
+
+/**
+ * igc_init_module - Driver Registration Routine
+ *
+ * igc_init_module is the first routine called when the driver is
+ * loaded. All it does is register with the PCI subsystem.
+ */
+static int __init igc_init_module(void)
+{
+ int ret;
+
+ pr_info("%s - version %s\n",
+ igc_driver_string, igc_driver_version);
+
+ pr_info("%s\n", igc_copyright);
+
+ ret = pci_register_driver(&igc_driver);
+ return ret;
Why the variable?
we can return 'pci_register_driver' here, but variable is more clearly.
+}
+
+module_init(igc_init_module);
+
+/**
+ * igc_exit_module - Driver Exit Cleanup Routine
+ *
+ * igc_exit_module is called just before the driver is removed
+ * from memory.
+ */
+static void __exit igc_exit_module(void)
+{
+ pci_unregister_driver(&igc_driver);
+}
+
+module_exit(igc_exit_module);
+/* igc_main.c */
I'd argue most editors make it fairly clear which file one is
editing, hence making this sort of comments entirely superfluous :)
Thanks for your comments.
