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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
