lkml.org 
[lkml]   [2012]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] LDT - Linux Driver Template
On Tue, Nov 13, 2012 at 08:46:37PM +0200, Constantine Shulyupin wrote:
> +++ b/samples/ltd/ldt.c
> @@ -0,0 +1,764 @@
> +/*
> + * LDT - Linux Driver Template
> + *
> + * Copyright (C) 2012 Constantine Shulyupin http://www.makelinux.net/
> + *
> + * Dual BSD/GPL License

That makes no sense for Linux-specific kernel code, why would you want
it to be dual licensed? Please fix this.

> + * Device Model (class, device)

Don't use class code in an example, it is slowly going away from the
whole kernel.

> +#define ctracer_cut_path(fn) (fn[0] != '/' ? fn : (strrchr(fn, '/') + 1))
> +#define __file__ ctracer_cut_path(__FILE__)

Why is this needed?

> +/*
> + * print_context prints execution context:
> + * hard interrupt, soft interrupt or scheduled task
> + */
> +
> +#define print_context() \
> + pr_debug("%s:%d %s %s 0x%x\n", __file__, __LINE__, __func__, \
> + (in_irq() ? "harirq" : current->comm), preempt_count());

Ick, no, never do that.

> +#define once(exp) do { \
> + static int _passed; if (!_passed) { exp; }; _passed = 1; } while (0)

We have macros for this already.

> +#define check(a) \
> + (ret = a, ((ret < 0) ? pr_warning("%s:%i %s FAIL\n\t%i=%s\n", \
> + __file__, __LINE__, __func__, ret, #a) : 0), ret)

Why?

> +#define pr_debug_hex(h) pr_debug("%s:%d %s %s = 0x%lX\n", \
> + __file__, __LINE__, __func__, #h, (long int)h)

This is not needed at all, just use the proper printk() attribute.

> +#define pr_debug_dec(d) pr_debug("%s:%d %s %s = %ld\n", \
> + __file__, __LINE__, __func__, #d, (long int)d)

Why?

> +#define pr_err_msg(m) pr_err("%s:%d %s %s\n", __file__, __LINE__, __func__, m)

Again, why?

Please don't create your own debugging macros, otherwise people will
copy them. Use the in-kernel ones, as they are the ones everyone should
use. And never use the __file__ things, that looks horrible when
building a kernel and makes no sense.

> +static char ldt_name[] = KBUILD_MODNAME;

Why not just use the macro itself?

> +static int bufsize = PFN_ALIGN(16 * 1024);

Why align?

> +static void *in_buf;
> +static void *out_buf;
> +static int uart_detected;
> +void *port_ptr;

Not static? I'm guessing you didn't run this through sparse?

> +static int port;
> +module_param(port, int, 0);
> +static int port_size;
> +module_param(port_size, int, 0);
> +
> +static int irq;
> +module_param(irq, int, 0);
> +
> +static int loopback;
> +module_param(loopback, int, 0);

No descriptions? Not good.

I've stopped here, I think this has a bunch more work in order to be a
"correct" example in the kernel source tree.

> +MODULE_LICENSE("Dual BSD/GPL");

Note that modules that touch driver core functions, like this one, can't
really be BSD licensed, sorry.

greg k-h


\
 
 \ /
  Last update: 2012-11-13 20:41    [W:0.117 / U:0.332 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site