RE: Updated FarSync driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dan,
    Thanks for the comments so far.  I'll start by working out how to spilt the patch up and then work on the individual comments.


Regards

Kevin

-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] 
Sent: 29 July 2013 12:43
To: Kevin Curtis
Cc: kernel-janitors@xxxxxxxxxxxxxxx; Dermot Smith
Subject: Re: Updated FarSync driver

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

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux