On Wed, 2013-09-18 at 11:12 +0100, Kevin Curtis wrote:
> Farsite Communications FarSync driver update
>
> Patch 6 of 7
>
> Introduce the fsflex driver.
> This driver is functionally equivalent to the farsync driver, and so
> can be used with the Generic HDLC, and ppp daemon.
>
> Signed-off-by: Kevin Curtis <kevin.curtis@xxxxxxxxxxx>
>
> ---
> diff -uprN -X linux-3.10.1/Documentation/dontdiff linux-3.10.1/drivers/net/wan/fsflex.c linux-3.10.1_new/drivers/net/wan/fsflex.c
> --- linux-3.10.1/drivers/net/wan/fsflex.c 1970-01-01 01:00:00.000000000 +0100
> +++ linux-3.10.1_new/drivers/net/wan/fsflex.c 2013-09-16 16:30:06.651104873 +0100
> @@ -0,0 +1,5693 @@
> +/*
> + * FarSync Flex driver for Linux (2.6.x kernel version)
Not exactly...
[...]
> +#define FST_MAX_CARDS 32
I don't think it's acceptable to set a static limit on number of devices
in a new driver.
[...]
> +int fst_txq_low = FST_LOW_WATER_MARK;
> +int fst_txq_high = FST_HIGH_WATER_MARK;
> +int fst_min_dma_len = FST_MIN_DMA_LEN;
> +int fst_dmathr = 0xdd00dd00;
> +int fst_iocinfo_version = FST_VERSION_CURRENT;
> +int fst_max_reads = 7;
> +int fst_excluded_cards;
> +int fst_excluded_list[FST_MAX_CARDS];
> +int fst_num_serials;
> +char *fst_serials[FST_MAX_CARDS];
All of which should be declared static, not implicitly extern. As
should all static variables and functions in this source file, as it's a
self-contained driver.
> +module_param(fst_txq_low, int, S_IRUGO);
> +module_param(fst_txq_high, int, S_IRUGO);
> +module_param(fst_min_dma_len, int, S_IRUGO);
> +module_param(fst_dmathr, int, S_IRUGO);
> +module_param(fst_iocinfo_version, int, S_IRUGO);
> +module_param(fst_max_reads, int, S_IRUGO);
> +module_param(fst_excluded_cards, int, S_IRUGO);
> +module_param_array(fst_excluded_list, int, NULL, S_IRUGO);
> +module_param_array(fst_serials, charp, &fst_num_serials, S_IRUGO);
Device configuration should be exposed through sysfs or ioctls, not
through module parameters.
[...]
> +#ifdef ARPHRD_RAWHDLC
> +#define ARPHRD_MYTYPE ARPHRD_RAWHDLC /* Raw frames */
> +#else
> +#define ARPHRD_MYTYPE ARPHRD_HDLC /* Cisco-HDLC (keepalives etc) */
> +#endif
#ifdef is not necessary in-tree.
[...]
> +#define usb_buffer_alloc usb_alloc_coherent
> +#define usb_buffer_free usb_free_coherent
Don't make up your own names for USB functions.
[...]
> +static int __init fst_init(void)
> +{
[...]
> + /* Create the /proc entry for /proc/fsflex
> + */
> + proc_create("fsflex", 0, NULL, &farsync_proc_fops);
[...]
Drivers should not be adding to the top level of /proc, and generally
should be adding any magic files under sysfs (per-device) or debugfs.
There is a /proc/driver directory if this information really doesn't fit
into either of those filesystems.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
--
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
