Re: Updated FarSync driver

Dan Carpenter
2013-07-29T11:42:45Z

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.