Re: Updated FarSync driver

[Dan Carpenter <dan.carpenter@oracle.com>]

Oh, in my previous email I forgot to say that the patches should go
to netdev with Dave Miller CC'd.

@@ -20,18 +20,29 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/version.h>
-#include <linux/pci.h>
+#include <linux/fs.h>
 #include <linux/sched.h>
-#include <linux/slab.h>
+#include <linux/proc_fs.h>
+#include <linux/pci.h>

There is no reason to shift the pci.h include around.  Don't do
that.

+#define FST_DRIVER_TYPE "WAN"
+
+

Don't put two blank lines in a row.

+module_param(fst_iocinfo_version, int, S_IRUGO);

I don't understand what this is for.  Probably it doesn't need to
be configurable.

Btw, here we are moving the module_param definitions around and
mostly they are unchanged.  When you are breaking the patch into
lots of patches that can be one of the early ones:

[patch 3/xx] farsight: move some definitions around

Moving the definitions in a separate patch is easy to review and it
makes the next patch (where you add in a few more definitions), it
makes that patch much smaller and easier to review as well.

+typedef struct class class_t;
+#define CLASS_DEVICE_CREATE(class, dev, device, fmt, rest) device_create(class, device, dev, NULL, fmt, rest)
 

checkpatch will complain about the class_t typedef, that's not how
typedefs are used in the kernel.  Also it's too generic a name.
Also don't do that #define just call device_create() directly.

/*      Card shared memory layout
  *      ============  */
-#pragma pack(1)
+#pragma pack(2)
 

Don't use #pragmas in the kernel.  Use the __packed macro to define
packed structs.

struct foo {
	char a;
	int b;
} __packed;

+typedef struct fst_fifo {
+	u16 fifo_length;
+	u16 read_idx;
+	u16 write_idx;
+	short overflow_idx;
+	u8 data[4];
+} FIFO, *PFIFO;

Typedef.  "FIFO" and "PFIFO" are terrible names and way too generic.
Use "struct fst_fifo" throughout.

+/*      Printing short cuts
+ */
+#define printk_err(fmt,A...)    printk ( KERN_ERR     FST_NAME ": " fmt, ## A )
+#define printk_warn(fmt,A...)   printk ( KERN_WARNING FST_NAME ": " fmt, ## A )
+#define printk_info(fmt,A...)   printk ( KERN_INFO    FST_NAME ": " fmt, ## A )
+

This seems like a compat layer.  Checkpatch will hopefully complain
about this.

/*
  *      PCI ID lookup table
  */
 static DEFINE_PCI_DEVICE_TABLE(fst_pci_dev_id) = {
-	{PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T2P, PCI_ANY_ID, 
-	 PCI_ANY_ID, 0, 0, FST_TYPE_T2P},
-
-	{PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T4P, PCI_ANY_ID, 
-	 PCI_ANY_ID, 0, 0, FST_TYPE_T4P},
-
-	{PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T1U, PCI_ANY_ID, 
-	 PCI_ANY_ID, 0, 0, FST_TYPE_T1U},
-
-	{PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T2U, PCI_ANY_ID, 
-	 PCI_ANY_ID, 0, 0, FST_TYPE_T2U},
-
-	{PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T4U, PCI_ANY_ID, 
-	 PCI_ANY_ID, 0, 0, FST_TYPE_T4U},
-
-	{PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_TE1, PCI_ANY_ID, 
-	 PCI_ANY_ID, 0, 0, FST_TYPE_TE1},
+	{
+#ifdef FSC_TXP_SUPPORT
+	PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T2P, PCI_ANY_ID,
+	PCI_ANY_ID, 0, 0, FST_TYPE_T2P}, {
+	PCI_VENDOR_ID_FARSITE, PCI_DEVICE_ID_FARSITE_T4P, PCI_ANY_ID,
+	PCI_ANY_ID, 0, 0, FST_TYPE_T4P}, {
+#endif

The original formatting was way better here.

Anyway, I feel like I have given you a lot to work on.

The good news is that this patch is mostly about adding a new driver
and that can be done in one patch.

I saw later on this driver creates a /proc file.  That should
probably sysfs file instead.  We don't add /proc files these days.

regards,
dan carpenter