Skip to contentSkip to conversation

LDT - Linux Driver Template

PATCH In review Constantine Shulyupin 6 participants 14 messages Nov 13, 2012 – Nov 16, 2012

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 LDT - Linux Driver Template

From: Constantine Shulyupin <[email protected]>

LDT is useful for Linux driver development beginners,
hackers and as starting point for a new drivers.
The driver uses following Linux facilities: module, platform driver, file operations (read/write, mmap, ioctl, blocking and nonblocking mode, polling), kfifo, completion, interrupt, tasklet, work, kthread, timer, simple misc device, multiple char devices, Device Model, configfs, UART 0x3f8,
HW loopback, SW loopback, ftracer.

---
 samples/Kconfig             |   14 +
 samples/Makefile            |    5 +-
 samples/ltd/Makefile        |    1 +
 samples/ltd/ldt.c           |  764 +++++++++++++++++++++++++++++++++++++++++++
 samples/ltd/ldt_plat_dev.c  |   68 ++++
 tools/testing/ldt/Makefile  |    6 +
 tools/testing/ldt/ctracer.h |  380 +++++++++++++++++++++
 tools/testing/ldt/dio.c     |  362 ++++++++++++++++++++
 tools/testing/ldt/ldt-test  |  142 ++++++++
 9 files changed, 1740 insertions(+), 2 deletions(-)
 create mode 100644 samples/ltd/Makefile
 create mode 100644 samples/ltd/ldt.c
 create mode 100644 samples/ltd/ldt_plat_dev.c
 create mode 100644 tools/testing/ldt/Makefile
 create mode 100644 tools/testing/ldt/ctracer.h
 create mode 100644 tools/testing/ldt/dio.c
 create mode 100755 tools/testing/ldt/ldt-test
samples/Kconfig +14 -0
index 7b6792a..2b93fd0 100644
@@ -69,4 +69,18 @@ config SAMPLE_RPMSG_CLIENT
 	  to communicate with an AMP-configured remote processor over
 	  the rpmsg bus.
 
+config SAMPLE_DRIVER_TEMPLATE
+	tristate "LDT - Linux driver template"
+	help
+	  Template of Linux device driver. Useful for Linux driver
+	  development beginners, hackers and as starting point for a new drivers.
+	  The driver uses following Linux facilities: module, platform driver,
+	  file operations (read/write, mmap, ioctl, blocking and nonblocking mode, polling),
+	  kfifo, completion, interrupt, tasklet, work, kthread, timer, simple misc device,
+	  multiple char devices, Device Model, configfs, UART 0x3f8, HW loopback,
+	  SW loopback, ftracer.
+	  Usermode test script and utility are located in tools/testing/ldt/
+
+	  List of more samples and skeletons can be found at http://elinux.org/Device_drivers
+
 endif # SAMPLES
samples/Makefile +3 -2
index 5ef08bb..d4b1818 100644
@@ -1,4 +1,5 @@
 # Makefile for Linux samples code
 
-obj-$(CONFIG_SAMPLES)	+= kobject/ kprobes/ tracepoints/ trace_events/ \
-			   hw_breakpoint/ kfifo/ kdb/ hidraw/ rpmsg/ seccomp/
+obj-$(CONFIG_SAMPLES)	+= kobject/ kprobes/ tracepoints/ trace_events/
+obj-$(CONFIG_SAMPLES)	+= hw_breakpoint/ kfifo/ kdb/ hidraw/ rpmsg/ seccomp/
+obj-$(CONFIG_SAMPLES)	+= ldt/
samples/ltd/Makefile new file +1 -0
new file mode 100644
index 0000000..efd691f
@@ -0,0 +1 @@
+obj-$(SAMPLE_DRIVER_TEMPLATE)+= ldt.o ldt_plat_dev.o
samples/ltd/ldt.c new file +764 -0

764 lines not on this page · raw message

samples/ltd/ldt_plat_dev.c new file +68 -0

68 lines not on this page · raw message

tools/testing/ldt/Makefile new file +6 -0
new file mode 100644
index 0000000..0bfefd6
@@ -0,0 +1,6 @@
+all: dio
+
+dio: CPPFLAGS+=-g -D CTRACER_ON
+
+clean:
+	rm -rf dio *.o  dio *.tmp *.log
tools/testing/ldt/ctracer.h new file +380 -0

380 lines not on this page · raw message

tools/testing/ldt/dio.c new file +362 -0

362 lines not on this page · raw message

tools/testing/ldt/ldt-test new file +142 -0

142 lines not on this page · raw message

Signature · 1 line
1.7.9.5
#1 lore raw

samples/ltd/ldt.c line 6

Quoted 8 lines · Constantine Shulyupin

+++ b/samples/ltd/ldt.c

samples/ltd/ldt.c new file +6 -0
@@ -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.

samples/ltd/ldt.c line 30

Quoted 1 line
samples/ltd/ldt.c new file +1 -0
+ *	Device Model (class, device)

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

Review line 9

Quoted 2 lines
(quoted diff) +2 -0
+#define ctracer_cut_path(fn) (fn[0] != '/' ? fn : (strrchr(fn, '/') + 1))
+#define __file__	ctracer_cut_path(__FILE__)

Why is this needed?

samples/ltd/ldt.c line 70

Quoted 8 lines
samples/ltd/ldt.c new file +8 -0
+/*
+ *	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.

samples/ltd/ldt.c line 73

Quoted 2 lines
samples/ltd/ldt.c new file +2 -0
+#define once(exp) do { \
+	static int _passed; if (!_passed) { exp; }; _passed = 1; } while (0)

We have macros for this already.

samples/ltd/ldt.c line 77

Quoted 3 lines
samples/ltd/ldt.c new file +3 -0
+#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?

samples/ltd/ldt.c line 80

Quoted 2 lines
samples/ltd/ldt.c new file +2 -0
+#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.

samples/ltd/ldt.c line 82

Quoted 2 lines
samples/ltd/ldt.c new file +2 -0
+#define pr_debug_dec(d)	pr_debug("%s:%d %s %s = %ld\n", \
+	__file__, __LINE__, __func__, #d, (long int)d)

Why?

samples/ltd/ldt.c line 84

Quoted 1 line
samples/ltd/ldt.c new file +1 -0
+#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.

samples/ltd/ldt.c line 87

Quoted 1 line
samples/ltd/ldt.c new file +1 -0
+static char ldt_name[] = KBUILD_MODNAME;

Why not just use the macro itself?

samples/ltd/ldt.c line 88

Quoted 1 line
samples/ltd/ldt.c new file +1 -0
+static int bufsize = PFN_ALIGN(16 * 1024);

Why align?

samples/ltd/ldt.c line 92

Quoted 4 lines
samples/ltd/ldt.c new file +4 -0
+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?

samples/ltd/ldt.c line 103

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+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.

Review line 44

Quoted 1 line
(quoted diff) +1 -0
+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

#2 lore raw
Quoted 3 lines
Quoted 1 line

+ * Device Model (class, device)

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

What to use instead class_create and device_create?

Thank you.

#3 lore raw
Quoted 4 lines · Constantine Shulyupin
Quoted 3 lines
Quoted 1 line

+ * Device Model (class, device)

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

What to use instead class_create and device_create?

What are you trying to do?

#4 lore raw
Quoted 7 lines · Greg KH
Quoted 4 lines · Constantine Shulyupin
Quoted 3 lines
Quoted 1 line

+ * Device Model (class, device)

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

What to use instead class_create and device_create?

What are you trying to do?

I trying to properly register char device and device region. Single char device could be registered as misc device with just misc_register. How to register properly char devices region?
Should to use device_register instead device_create to create dev files? Which other registration functions char device must to call besides alloc_chrdev_region, cdev_add?

#5 lore raw
Quoted 11 lines · Constantine Shulyupin
Quoted 7 lines · Greg KH
Quoted 4 lines · Constantine Shulyupin
Quoted 3 lines
Quoted 1 line

+ * Device Model (class, device)

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

What to use instead class_create and device_create?

What are you trying to do?

I trying to properly register char device and device region. Single char device could be registered as misc device with just misc_register.

When you do that, the struct device is automatically registered with the system, why do you need to do it again?

Quoted 1 line

How to register properly char devices region?

Using the proper calls, which are not the misc_register ones :)

Quoted 1 line

Should to use device_register instead device_create to create dev files?

Depends on what you want to do :)

Quoted 2 lines

Which other registration functions char device must to call besides alloc_chrdev_region, cdev_add?

Again, I think you are mixing two different things here. You can't mix the char interface with the misc_register interface, they don't play well, it's either one or the other.

Now I agree using the char interface isn't the most "obvious" and I have a set of ideas/half-baked patches floating around that aim to clean it up, but for now, I'd recommend just using the misc interface, it's worlds simpler, makes sense, and handles all of the struct device work for you automatically.

thanks,

greg k-h

#6 lore raw
Quoted 5 lines

Now I agree using the char interface isn't the most "obvious" and I have a set of ideas/half-baked patches floating around that aim to clean it up, but for now, I'd recommend just using the misc interface, it's worlds simpler, makes sense, and handles all of the struct device work for you automatically.

OK. Thanks.
Originally I've used only misc device and introduced dev region up on request. But it makes code too complicated.
I also prefer to use only misc device in LDT.

Thank you

#7 lore raw
In reply to Greg KH
Quoted 4 lines · Greg KH

samples/ltd/ldt.c line 80

Quoted 2 lines
samples/ltd/ldt.c new file +2 -0
+#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.

Macro above allows tidy tracing code:

        pr_debug_hex(ioread8(port_ptr + UART_IER));
        pr_debug_hex(ioread8(port_ptr + UART_IIR));
        pr_debug_hex(ioread8(port_ptr + UART_FCR));
        pr_debug_hex(ioread8(port_ptr + UART_LCR));
        pr_debug_hex(ioread8(port_ptr + UART_MCR));
        pr_debug_hex(ioread8(port_ptr + UART_LSR));
        pr_debug_hex(ioread8(port_ptr + UART_MSR));

Without that macro, code above should be rewritten with pr_debug (or printk) as:

        pr_debug("UART_IER=0x%02X\n", ioread8(port_ptr + UART_IER));
        pr_debug("UART_IIR=0x%02X\n", ioread8(port_ptr + UART_IIR));
        pr_debug("UART_FCR=0x%02X\n", ioread8(port_ptr + UART_FCR));
        pr_debug("UART_LCR=0x%02X\n", ioread8(port_ptr + UART_LCR));
        pr_debug("UART_MCR=0x%02X\n", ioread8(port_ptr + UART_MCR));
        pr_debug("UART_LSR=0x%02X\n", ioread8(port_ptr + UART_LSR));
        pr_debug("UART_MSR=0x%02X\n", ioread8(port_ptr + UART_MSR));

That is less readable and less supportable.

I prefer the fist case.

Actually I use a lot shorter macro:
#define traceh(h) printk("%s = 0x%lX\n", #h, (long int)h)

What is you opinion? Which method is better?
Thank you.

#8 lore raw
Quoted 26 lines · Constantine Shulyupin
Quoted 4 lines · Greg KH

samples/ltd/ldt.c line 80

Quoted 2 lines
samples/ltd/ldt.c new file +2 -0
+#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.

Macro above allows tidy tracing code:

        pr_debug_hex(ioread8(port_ptr + UART_IER));
        pr_debug_hex(ioread8(port_ptr + UART_IIR));
        pr_debug_hex(ioread8(port_ptr + UART_FCR));
        pr_debug_hex(ioread8(port_ptr + UART_LCR));
        pr_debug_hex(ioread8(port_ptr + UART_MCR));
        pr_debug_hex(ioread8(port_ptr + UART_LSR));
        pr_debug_hex(ioread8(port_ptr + UART_MSR));

Without that macro, code above should be rewritten with pr_debug (or printk) as:

        pr_debug("UART_IER=0x%02X\n", ioread8(port_ptr + UART_IER));
        pr_debug("UART_IIR=0x%02X\n", ioread8(port_ptr + UART_IIR));
        pr_debug("UART_FCR=0x%02X\n", ioread8(port_ptr + UART_FCR));
        pr_debug("UART_LCR=0x%02X\n", ioread8(port_ptr + UART_LCR));
        pr_debug("UART_MCR=0x%02X\n", ioread8(port_ptr + UART_MCR));
        pr_debug("UART_LSR=0x%02X\n", ioread8(port_ptr + UART_LSR));
        pr_debug("UART_MSR=0x%02X\n", ioread8(port_ptr + UART_MSR));
Quoted 1 line

That is less readable and less supportable.

What better is to not do foolish stuff like this at all :)

If you need tracing, use the in-kernel tracing framework, don't roll your own.

Quoted 1 line

I prefer the fist case.

I prefer the flower case :)

Quoted 4 lines

Actually I use a lot shorter macro:
#define traceh(h) printk("%s = 0x%lX\n", #h, (long int)h)

What is you opinion? Which method is better?

Again, neither, don't clutter your code up with unneeded things like this.

greg k-h

#9 lore raw
In reply to Constantine Shulyupin

My apologies. As celinux-dev list administrator, I messed up my moderation and this response from Greg got discarded instead of sent to the list.

Here it is, sent by me on Greg's behalf...
-- Tim

Quoted 26 lines · Constantine Shulyupin
Quoted 4 lines · Greg KH

samples/ltd/ldt.c line 80

Quoted 2 lines
samples/ltd/ldt.c new file +2 -0
+#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.

Macro above allows tidy tracing code:

        pr_debug_hex(ioread8(port_ptr + UART_IER));
        pr_debug_hex(ioread8(port_ptr + UART_IIR));
        pr_debug_hex(ioread8(port_ptr + UART_FCR));
        pr_debug_hex(ioread8(port_ptr + UART_LCR));
        pr_debug_hex(ioread8(port_ptr + UART_MCR));
        pr_debug_hex(ioread8(port_ptr + UART_LSR));
        pr_debug_hex(ioread8(port_ptr + UART_MSR));

Without that macro, code above should be rewritten with pr_debug (or printk) as:

        pr_debug("UART_IER=0x%02X\n", ioread8(port_ptr + UART_IER));
        pr_debug("UART_IIR=0x%02X\n", ioread8(port_ptr + UART_IIR));
        pr_debug("UART_FCR=0x%02X\n", ioread8(port_ptr + UART_FCR));
        pr_debug("UART_LCR=0x%02X\n", ioread8(port_ptr + UART_LCR));
        pr_debug("UART_MCR=0x%02X\n", ioread8(port_ptr + UART_MCR));
        pr_debug("UART_LSR=0x%02X\n", ioread8(port_ptr + UART_LSR));
        pr_debug("UART_MSR=0x%02X\n", ioread8(port_ptr + UART_MSR));
Quoted 1 line

That is less readable and less supportable.

What better is to not do foolish stuff like this at all

If you need tracing, use the in-kernel tracing framework, don't roll your own.

Quoted 1 line

I prefer the fist case.

I prefer the flower case :)

Quoted 4 lines

Actually I use a lot shorter macro:
#define traceh(h) printk("%s = 0x%lX\n", #h, (long int)h)

What is you opinion? Which method is better?

Again, neither, don't clutter your code up with unneeded things like this.

greg k-h

#10 lore raw
In reply to Constantine Shulyupin
Quoted 11 lines · Constantine Shulyupin

From: Constantine Shulyupin <[email protected]>

LDT is useful for Linux driver development beginners,
hackers and as starting point for a new drivers.
The driver uses following Linux facilities: module, platform driver, file operations (read/write, mmap, ioctl, blocking and nonblocking mode, polling), kfifo, completion, interrupt, tasklet, work, kthread, timer, simple misc device, multiple char devices, Device Model, configfs, UART 0x3f8,
HW loopback, SW loopback, ftracer.

In general, since this is intended to be example code for beginner developers, the code should have more comments explaining what each part does. It should also try as hard as possible to conform to kernel coding standards. I would suggest running it through checkpatch and sparse, since those tools are often recommended to (new) kernel developers.

Some more comments below.

~Ryan

Quoted 141 lines
---
 samples/Kconfig             |   14 +
 samples/Makefile            |    5 +-
 samples/ltd/Makefile        |    1 +
 samples/ltd/ldt.c           |  764 +++++++++++++++++++++++++++++++++++++++++++
 samples/ltd/ldt_plat_dev.c  |   68 ++++
 tools/testing/ldt/Makefile  |    6 +
 tools/testing/ldt/ctracer.h |  380 +++++++++++++++++++++
 tools/testing/ldt/dio.c     |  362 ++++++++++++++++++++
 tools/testing/ldt/ldt-test  |  142 ++++++++
 9 files changed, 1740 insertions(+), 2 deletions(-)
 create mode 100644 samples/ltd/Makefile
 create mode 100644 samples/ltd/ldt.c
 create mode 100644 samples/ltd/ldt_plat_dev.c
 create mode 100644 tools/testing/ldt/Makefile
 create mode 100644 tools/testing/ldt/ctracer.h
 create mode 100644 tools/testing/ldt/dio.c
 create mode 100755 tools/testing/ldt/ldt-test
samples/Kconfig +14 -0
index 7b6792a..2b93fd0 100644
@@ -69,4 +69,18 @@ config SAMPLE_RPMSG_CLIENT
 	  to communicate with an AMP-configured remote processor over
 	  the rpmsg bus.
 
+config SAMPLE_DRIVER_TEMPLATE
+	tristate "LDT - Linux driver template"
+	help
+	  Template of Linux device driver. Useful for Linux driver
+	  development beginners, hackers and as starting point for a new drivers.
+	  The driver uses following Linux facilities: module, platform driver,
+	  file operations (read/write, mmap, ioctl, blocking and nonblocking mode, polling),
+	  kfifo, completion, interrupt, tasklet, work, kthread, timer, simple misc device,
+	  multiple char devices, Device Model, configfs, UART 0x3f8, HW loopback,
+	  SW loopback, ftracer.
+	  Usermode test script and utility are located in tools/testing/ldt/
+
+	  List of more samples and skeletons can be found at http://elinux.org/Device_drivers
+
 endif # SAMPLES
samples/Makefile +3 -2
index 5ef08bb..d4b1818 100644
@@ -1,4 +1,5 @@
 # Makefile for Linux samples code
 
-obj-$(CONFIG_SAMPLES)	+= kobject/ kprobes/ tracepoints/ trace_events/ \
-			   hw_breakpoint/ kfifo/ kdb/ hidraw/ rpmsg/ seccomp/
+obj-$(CONFIG_SAMPLES)	+= kobject/ kprobes/ tracepoints/ trace_events/
+obj-$(CONFIG_SAMPLES)	+= hw_breakpoint/ kfifo/ kdb/ hidraw/ rpmsg/ seccomp/
+obj-$(CONFIG_SAMPLES)	+= ldt/
samples/ltd/Makefile new file +1 -0
new file mode 100644
index 0000000..efd691f
@@ -0,0 +1 @@
+obj-$(SAMPLE_DRIVER_TEMPLATE)+= ldt.o ldt_plat_dev.o
samples/ltd/ldt.c new file +74 -0
new file mode 100644
index 0000000..a4d2b3b
@@ -0,0 +1,764 @@
+/*
+ *	LDT - Linux Driver Template
+ *
+ *	Copyright (C) 2012 Constantine Shulyupin http://www.makelinux.net/
+ *
+ *	Dual BSD/GPL License
+ *
+ *
+ *	The driver demonstrates usage of following Linux facilities:
+ *
+ *	Linux kernel module
+ *	file_operations
+ *		read and write (UART)
+ *		blocking read
+ *		polling
+ *		mmap
+ *		ioctl
+ *	kfifo
+ *	completion
+ *	interrupt
+ *	tasklet
+ *	timer
+ *	work
+ *	kthread
+ *	simple single misc device file (miscdevice, misc_register)
+ *	multiple char device files (alloc_chrdev_region)
+ *	debugfs
+ *	platform_driver and platform_device in another module
+ *	simple UART driver on port 0x3f8 with IRQ 4
+ *	Device Model (class, device)
+ *	Power Management (dev_pm_ops)
+ *	Device Tree (of_device_id)
+ *
+ *	TODO:
+ *	linked list
+ *	private instance state struct
+ *
+ */
+
+#include <linux/io.h>
+#include <linux/mm.h>
+#include <linux/interrupt.h>
+#include <linux/sched.h>
+#include <linux/delay.h>
+#include <linux/kthread.h>
+#include <linux/timer.h>
+#include <linux/kfifo.h>
+#include <linux/fs.h>
+#include <linux/poll.h>
+#include <linux/module.h>
+#include <linux/miscdevice.h>
+#include <linux/platform_device.h>
+#include <linux/serial_reg.h>
+#include <linux/debugfs.h>
+#include <linux/cdev.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/mod_devicetable.h>
+
+#define ctracer_cut_path(fn) (fn[0] != '/' ? fn : (strrchr(fn, '/') + 1))
+#define __file__	ctracer_cut_path(__FILE__)
+
+/*
+ *	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());
+
+#define once(exp) do { \
+	static int _passed; if (!_passed) { exp; }; _passed = 1; } while (0)
+

samples/ltd/ldt.c line 84

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+#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)
+
+#define pr_debug_hex(h)	pr_debug("%s:%d %s %s = 0x%lX\n", \
+	__file__, __LINE__, __func__, #h, (long int)h)
+#define pr_debug_dec(d)	pr_debug("%s:%d %s %s = %ld\n", \
+	__file__, __LINE__, __func__, #d, (long int)d)
+
+#define pr_err_msg(m)	pr_err("%s:%d %s %s\n", __file__, __LINE__, __func__, m)

This is all extensively ugly.

samples/ltd/ldt.c line 92

Quoted 7 lines
samples/ltd/ldt.c new file +7 -0
+
+static char ldt_name[] = KBUILD_MODNAME;
+static int bufsize = PFN_ALIGN(16 * 1024);
+static void *in_buf;
+static void *out_buf;
+static int uart_detected;
+void *port_ptr;

Having globals like this means that only one instance of the device can be present, which is not true for many devices, and sets a bad example for copy-pasters, which this is aimed at. The typical way to do this is to have a struct:

	struct ldt_info {
		void *in_buf;
		void *out_buf;
		...
	};

And then allocate it in probe, and use dev_set_drvdata or similar to associate it with the device.

Quoted 20 lines
samples/ltd/ldt.c new file +20 -0
+
+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);
+
+static int isr_counter;
+static int ldt_work_counter;
+
+#define FIFO_SIZE 128		/* should be power of two */
+static DEFINE_KFIFO(in_fifo, char, FIFO_SIZE);
+static DEFINE_KFIFO(out_fifo, char, FIFO_SIZE);
+
+static DECLARE_WAIT_QUEUE_HEAD(ldt_readable);

samples/ltd/ldt.c line 122

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+
+static spinlock_t fifo_lock;
+
+
+/*
+ *	ldt_received - called with data received from HW port
+ *	Called from tasklet, which is fired from ISR or timer
+ */
+
+static void ldt_received(void *data, int size)

Why the indents on the comments? Also, remove the blank lines between the comments and the first line of the function.

Since this is example code, it should probably use proper kerneldoc formatted comments.

Quoted 8 lines
samples/ltd/ldt.c new file +8 -0
+{
+	kfifo_in_spinlocked(&in_fifo, data, size, &fifo_lock);
+	wake_up_interruptible(&ldt_readable);
+}
+
+/*
+ *	ldt_send - send data to HW port or emulated SW loopback
+ */

samples/ltd/ldt.c line 140

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+
+static void ldt_send(void *data, int size)
+{
+	if (uart_detected) {
+		if (ioread8(port_ptr + UART_LSR) & UART_LSR_THRE)
+			iowrite8(*(char *)data, port_ptr + UART_TX);
+		else
+			pr_err_msg("overflow");
+	} else
+		/* emulate loopback  */

Braces should be used for single lines on the 'else' if the 'if' part also need braces. Note that comments are not statements, so the indentation here is misleading, it is actually interpreted as:

	if (uart_detected) {
		...
	} else if (loopback) {
		...
	}

Which may not be what you want (and certainly isn't what the code layout suggests).

samples/ltd/ldt.c line 147

Quoted 7 lines
samples/ltd/ldt.c new file +7 -0
+	if (loopback)
+		ldt_received(data, size);
+}
+
+/*
+ *	work
+ */

Useless comment.

samples/ltd/ldt.c line 155

Quoted 8 lines
samples/ltd/ldt.c new file +8 -0
+
+static void ldt_work_func(struct work_struct *work)
+{
+	once(print_context());
+	ldt_work_counter++;
+}
+
+DECLARE_WORK(ldt_work, ldt_work_func);

This should have static to stop it polluting the global namespace. Even better, it should be moved into a priv struct. Also, all global data should really be declared at the top of the file, unless there is a good reason not to do so.

samples/ltd/ldt.c line 160

Quoted 5 lines
samples/ltd/ldt.c new file +5 -0
+
+/*
+ *	tasklet
+ */
+

More useless comments.

samples/ltd/ldt.c line 164

Quoted 4 lines
samples/ltd/ldt.c new file +4 -0
+static DECLARE_COMPLETION(ldt_complete);
+
+#define tx_ready()	(ioread8(port_ptr + UART_LSR) & UART_LSR_THRE)
+#define rx_ready()	(ioread8(port_ptr + UART_LSR) & UART_LSR_DR)

Stuff like this should be functions, not macros.

samples/ltd/ldt.c line 168

Quoted 4 lines
samples/ltd/ldt.c new file +4 -0
+
+static void ldt_tasklet_func(unsigned long d)
+{
+	char data_out, data_in;

Blank line between variable declarations and code.

samples/ltd/ldt.c line 171

Quoted 3 lines
samples/ltd/ldt.c new file +3 -0
+	once(print_context());
+	if (uart_detected) {
+		while (tx_ready() && kfifo_out_spinlocked(&out_fifo, &data_out, sizeof(data_out), &fifo_lock)) {

Lines should be split to fit inside 80 columns.

Quoted 86 lines
samples/ltd/ldt.c new file +86 -0
+			pr_debug_hex(ioread8(port_ptr + UART_LSR));
+			pr_debug_dec(data_out);
+			if (data_out >= 32)
+				pr_debug("data_out = '%c' ", data_out);
+			ldt_send(&data_out, sizeof(data_out));
+		}
+		while (rx_ready()) {
+			pr_debug_hex(ioread8(port_ptr + UART_LSR));
+			data_in = ioread8(port_ptr + UART_RX);
+			pr_debug_dec(data_in);
+			if (data_in >= 32)
+				pr_debug("data_out = '%c' ", data_in);
+			ldt_received(&data_in, sizeof(data_in));
+		}
+	} else {
+		while (kfifo_out_spinlocked(&out_fifo, &data_out, sizeof(data_out), &fifo_lock)) {
+			pr_debug_dec(data_out);
+			ldt_send(&data_out, sizeof(data_out));
+		}
+	}
+	schedule_work(&ldt_work);
+	complete(&ldt_complete);
+}
+
+static DECLARE_TASKLET(ldt_tasklet, ldt_tasklet_func, 0);
+
+/*
+ *	interrupt
+ */
+
+static irqreturn_t ldt_isr(int irq, void *dev_id, struct pt_regs *regs)
+{
+	/*
+	 *      UART interrupt is not fired in loopback mode,
+	 *      therefore fire ldt_tasklet from timer too
+	 */
+	once(print_context());
+	isr_counter++;
+	pr_debug_hex(ioread8(port_ptr + UART_FCR));
+	pr_debug_hex(ioread8(port_ptr + UART_IIR));
+	tasklet_schedule(&ldt_tasklet);
+	return IRQ_HANDLED;	/* our IRQ */
+}
+
+/*
+ *	timer
+ */
+
+static struct timer_list ldt_timer;
+static void ldt_timer_func(unsigned long data)
+{
+	/*
+	 *      this timer is used just to fire ldt_tasklet,
+	 *      when there is no interrupt in loopback mode
+	 */
+	if (loopback)
+		tasklet_schedule(&ldt_tasklet);
+	mod_timer(&ldt_timer, jiffies + HZ / 100);
+}
+
+static DEFINE_TIMER(ldt_timer, ldt_timer_func, 0, 0);
+
+/*
+ *	file_operations
+ */
+
+static int ldt_open(struct inode *inode, struct file *file)
+{
+	print_context();
+	pr_debug_dec(imajor(inode));
+	pr_debug_dec(iminor(inode));
+	pr_debug_hex(file->f_flags & O_NONBLOCK);
+	return 0;
+}
+
+static int ldt_release(struct inode *inode, struct file *file)
+{
+	print_context();
+	pr_debug_dec(imajor(inode));
+	pr_debug_dec(iminor(inode));
+	pr_debug_dec(isr_counter);
+	pr_debug_dec(ldt_work_counter);
+	return 0;
+}
+
+/*

samples/ltd/ldt.c line 267

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+ *	read
+ */
+
+static DEFINE_MUTEX(read_lock);
+
+static ssize_t ldt_read(struct file *file, char __user * buf, size_t count, loff_t * ppos)
+{
+	int ret;
+	unsigned int copied;
+	if (kfifo_is_empty(&in_fifo)) {

Doesn't this require locking against whatever is filling the fifo?

samples/ltd/ldt.c line 270

Quoted 3 lines
samples/ltd/ldt.c new file +3 -0
+		if (file->f_flags & O_NONBLOCK) {
+			ret = -EAGAIN;
+			goto exit;
	return -EAGAIN;

samples/ltd/ldt.c line 277

Quoted 7 lines
samples/ltd/ldt.c new file +7 -0
+		} else {
+			pr_err_msg("waiting");
+			ret = wait_event_interruptible(ldt_readable, !kfifo_is_empty(&in_fifo));
+			if (ret == -ERESTARTSYS) {
+				pr_err_msg("interrupted");
+				ret = -EINTR;
+				goto exit;
	return -EINTR;

samples/ltd/ldt.c line 286

Quoted 9 lines
samples/ltd/ldt.c new file +9 -0
+			}
+		}
+	}
+	if (mutex_lock_interruptible(&read_lock)) {
+		pr_err_msg("interrupted");
+		return -EINTR;
+	}
+	ret = kfifo_to_user(&in_fifo, buf, count, &copied);
+	mutex_unlock(&read_lock);

The read_lock is only used here, so would only protect against concurrent reads. Isn't the read for a device function already synchronised against itself by the caller?

Quoted 11 lines
samples/ltd/ldt.c new file +11 -0
+exit:
+	return ret ? ret : copied;
+}
+
+/*
+ *	write
+ */
+
+static DEFINE_MUTEX(write_lock);
+
+static ssize_t ldt_write(struct file *file, const char __user * buf, size_t count, loff_t * ppos)

samples/ltd/ldt.c line 307

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+{
+	int ret;
+	unsigned int copied;
+	/* TODO: wait_event_interruptible ... ldt_writeable */
+	if (mutex_lock_interruptible(&write_lock))
+		return -EINTR;
+	ret = kfifo_from_user(&out_fifo, buf, count, &copied);
+	mutex_unlock(&write_lock);
+	tasklet_schedule(&ldt_tasklet);
+	return ret ? ret : copied;

Shouldn't this function be grabbing fifo_lock to prevent concurrent access to the fifo by the tasklet function? write_lock has the same issues described for read_lock above.

samples/ltd/ldt.c line 320

Quoted 13 lines
samples/ltd/ldt.c new file +13 -0
+}
+
+/*
+ *	polling
+ */
+
+static unsigned int ldt_poll(struct file *file, poll_table * pt)
+{
+	unsigned int mask = 0;
+	poll_wait(file, &ldt_readable, pt);
+	/*poll_wait(file, ldt_writeable, pt); TODO */
+
+	if (!kfifo_is_empty(&in_fifo))

Locking?

samples/ltd/ldt.c line 326

Quoted 6 lines
samples/ltd/ldt.c new file +6 -0
+		mask |= POLLIN | POLLRDNORM;
+	mask |= POLLOUT | POLLWRNORM;
+#if 0
+	mask |= POLLHUP;	/* on output eof */
+	mask |= POLLERR;	/* on output error */
+#endif

TODO comments, and #if 0 blocks are extremly confusing in example code, especially when they lack any real explanation.

samples/ltd/ldt.c line 337

Quoted 11 lines
samples/ltd/ldt.c new file +11 -0
+	pr_debug_hex(mask);
+	return mask;
+}
+
+/*
+ *	pages_flag - set or clear a flag for sequence of pages
+ *
+ *	more generic solution instead SetPageReserved, ClearPageReserved etc
+ */
+
+void pages_flag(struct page *page, int pages, int mask, int value)

Should be static. I don't think this function should exists, especially not as example code.

samples/ltd/ldt.c line 338

Quoted 2 lines
samples/ltd/ldt.c new file +2 -0
+{
+	for (; pages; pages--, page++)

page and pages is really confusing. Maybe page and num_pages would be better?

Quoted 39 lines
samples/ltd/ldt.c new file +39 -0
+		if (value)
+			__set_bit(mask, &page->flags);
+		else
+			__clear_bit(mask, &page->flags);
+}
+
+/*
+ *	mmap
+ */
+static int ldt_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+	void *buf = NULL;
+	if (vma->vm_flags & VM_WRITE)
+		buf = in_buf;
+	else if (vma->vm_flags & VM_READ)
+		buf = out_buf;
+	if (!buf)
+		return -EINVAL;
+	if (remap_pfn_range(vma, vma->vm_start, virt_to_phys(buf) >> PAGE_SHIFT,
+			    vma->vm_end - vma->vm_start, vma->vm_page_prot)) {
+		pr_err_msg("remap_pfn_range failed");
+		return -EAGAIN;
+	}
+	return 0;
+}
+
+/*
+ *	ioctl
+ */
+
+#define trace_ioctl(nr) printk("ioctl=(%c%c %c #%i %i)\n", \
+	(_IOC_READ & _IOC_DIR(nr)) ? 'r' : ' ', (_IOC_WRITE & _IOC_DIR(nr)) ? 'w' : ' ', \
+	_IOC_TYPE(nr), _IOC_NR(nr), _IOC_SIZE(nr))
+
+static long ldt_ioctl(struct file *f, unsigned int cmnd, unsigned long arg)
+{
+	void __user *user = (void *)arg;
+	pr_debug_hex(cmnd);
+	pr_debug_hex(arg);

samples/ltd/ldt.c line 388

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+	trace_ioctl(cmnd);
+	if (_IOC_DIR(cmnd) == _IOC_WRITE) {
+		copy_from_user(in_buf, user, _IOC_SIZE(cmnd));
+		memcpy(out_buf, in_buf, bufsize);
+		memset(in_buf, 0, bufsize);
+	}
+	if (_IOC_DIR(cmnd) == _IOC_READ) {
+		copy_to_user(user, out_buf, _IOC_SIZE(cmnd));
+		memset(out_buf, 0, bufsize);
+	}

Locking for the kfifo accesses?

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+	switch (_IOC_TYPE(cmnd)) {
+	case 'A':
+		switch (_IOC_NR(cmnd)) {
+		case 0:
+			break;
+		}
+		break;
+	}
+	return 0;
+}

samples/ltd/ldt.c line 408

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+
+static const struct file_operations ldt_fops = {
+	.owner = THIS_MODULE,
+	.open = ldt_open,
+	.release = ldt_release,
+	.read = ldt_read,
+	.write = ldt_write,
+	.poll = ldt_poll,
+	.mmap = ldt_mmap,
+	.unlocked_ioctl = ldt_ioctl,

Tabify would be nice:

	.owner		= THIS_MODULE,
	.open		= ldt_open,
	.release	= ldt_release,
	...

samples/ltd/ldt.c line 411

Quoted 3 lines
samples/ltd/ldt.c new file +3 -0
+};
+
+#ifdef USE_MISCDEV

Should be a Kconfig option?

samples/ltd/ldt.c line 419

Quoted 8 lines
samples/ltd/ldt.c new file +8 -0
+/*
+ *	use miscdevice for single instance device
+ */
+static struct miscdevice ldt_miscdev = {
+	MISC_DYNAMIC_MINOR,
+	ldt_name,
+	&ldt_fops,
+};

Use a C99 initialiser.

samples/ltd/ldt.c line 426

Quoted 7 lines
samples/ltd/ldt.c new file +7 -0
+#else
+/*
+ *	used cdev and device for multiple instances device
+ */
+
+static int devs = 8;
+module_param(devs, int, 0);

MODULE_PARM_DESC?

samples/ltd/ldt.c line 439

Quoted 13 lines
samples/ltd/ldt.c new file +13 -0
+
+static struct cdev ldt_cdev;
+static struct class *ldt_class;
+static struct device *ldt_dev;
+#if 0
+static char *ldt_devnode(struct device *dev, umode_t * mode)
+{
+	if (mode)
+		*mode = S_IRUGO | S_IWUGO;
+	/* *mode = 0666; */
+	return NULL;
+}
+#endif

Why is this commented out?

Quoted 4 lines
samples/ltd/ldt.c new file +4 -0
+#endif
+
+/*
+ *	kthread

samples/ltd/ldt.c line 453

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+ */
+
+static int ldt_thread_sub(void *data)
+{
+	int ret = 0;
+	/*
+	   perform here a useful work in task context
+	 */
+	return ret;
+}

This seems fairly pointless. The ldt_thread function is the important bit. A simple thread might do all of its processing there. This implies that a sub-function is always required.

Quoted 19 lines
samples/ltd/ldt.c new file +19 -0
+
+static int ldt_thread(void *data)
+{
+	int ret = 0;
+	print_context();
+	allow_signal(SIGINT);
+	while (!kthread_should_stop()) {
+		ret = wait_for_completion_interruptible(&ldt_complete);
+		if (ret == -ERESTARTSYS) {
+			pr_err_msg("interrupted");
+			ret = -EINTR;
+			break;
+		}
+		ret = ldt_thread_sub(data);
+	}
+	return ret;
+}
+
+/*

samples/ltd/ldt.c line 482

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+ *	UART
+ */
+
+static struct resource *port_r;
+
+static int uart_probe(void)
+{
+	int ret = 0;
+	if (port) {
+		port_ptr = ioport_map(port, port_size);

Error checking?

samples/ltd/ldt.c line 486

Quoted 4 lines
samples/ltd/ldt.c new file +4 -0
+		pr_debug_hex(port_ptr);
+		port_r = request_region(port, port_size, ldt_name);
+		pr_debug_hex(port_r);
+		/* ignore error */

Why ignore errors? This just encourages driver developers using this example to do the same.

samples/ltd/ldt.c line 489

Quoted 3 lines
samples/ltd/ldt.c new file +3 -0
+	}
+	if (irq) {
+		ret = check(request_irq(irq, (void *)ldt_isr, IRQF_SHARED, ldt_name, THIS_MODULE));

Get rid of the check macro and do some proper error checking. Why is the second argument being cast as void *?

samples/ltd/ldt.c line 501

Quoted 12 lines
samples/ltd/ldt.c new file +12 -0
+		iowrite8(UART_MCR_RTS | UART_MCR_OUT2 | UART_MCR_LOOP, port_ptr + UART_MCR);
+		uart_detected = (ioread8(port_ptr + UART_MSR) & 0xF0) == (UART_MSR_DCD | UART_MSR_CTS);
+		pr_debug_hex(ioread8(port_ptr + UART_MSR));
+
+		if (uart_detected) {
+			/*iowrite8(UART_IER_MSI | UART_IER_THRI |  UART_IER_RDI | UART_IER_RLSI, port_ptr + UART_IER); */
+			iowrite8(UART_IER_RDI | UART_IER_RLSI | UART_IER_THRI, port_ptr + UART_IER);
+			iowrite8(UART_MCR_DTR | UART_MCR_RTS | UART_MCR_OUT2, port_ptr + UART_MCR);
+			iowrite8(UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT, port_ptr + UART_FCR);
+			pr_debug_dec(loopback);
+			if (loopback)
+				iowrite8(ioread8(port_ptr + UART_MCR) | UART_MCR_LOOP, port_ptr + UART_MCR);

Break these lines up to get them under 80 columns.

Quoted 15 lines
samples/ltd/ldt.c new file +15 -0
+		}
+		if (!uart_detected && loopback) {
+			pr_warn("Emulating loopback in software\n");
+			ret = -ENODEV;
+		}
+	}
+	pr_debug_hex(uart_detected);
+	pr_debug_hex(ioread8(port_ptr + UART_IER));
+	pr_debug_hex(ioread8(port_ptr + UART_IIR));
+	pr_debug_hex(ioread8(port_ptr + UART_FCR));
+	pr_debug_hex(ioread8(port_ptr + UART_LCR));
+	pr_debug_hex(ioread8(port_ptr + UART_MCR));
+	pr_debug_hex(ioread8(port_ptr + UART_LSR));
+	pr_debug_hex(ioread8(port_ptr + UART_MSR));
+	return ret;

samples/ltd/ldt.c line 526

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+}
+
+static struct task_struct *thread;
+static struct dentry *debugfs;
+static int major;
+
+int chrdev_region_init(char *dev_name)
+{
+	int ret;
+	int d;

Same type variables can go on one line.

Quoted 19 lines
samples/ltd/ldt.c new file +19 -0
+	dev_t devid;
+	devid = MKDEV(major, 0);
+	ret = check(alloc_chrdev_region(&devid, 0, devs, dev_name));
+	major = MAJOR(devid);
+	pr_debug_dec(major);
+	cdev_init(&ldt_cdev, &ldt_fops);
+	check(cdev_add(&ldt_cdev, MKDEV(major, 0), devs));
+	ldt_class = class_create(THIS_MODULE, dev_name);
+	/* ldt_class->devnode = ldt_devnode; */
+	ldt_dev = device_create(ldt_class, NULL, devid, NULL, "%s", dev_name);
+	for (d = 1; d < devs; d++)
+		device_create(ldt_class, NULL, MKDEV(major, d), NULL, "%s%d", dev_name, d);
+	pr_debug_dec(IS_ERR(ldt_dev));
+	pr_debug_hex(ldt_dev);
+	return major;
+}
+
+/*
+ *	ldt_probe - main initialization function

samples/ltd/ldt.c line 555

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+ */
+
+static __devinit int ldt_probe(struct platform_device *pdev)
+{
+	int ret;
+	char *data = NULL;
+	struct resource *r;
+	print_context();
+	printk(KERN_DEBUG"%s %s %s", ldt_name, __DATE__, __TIME__);
+	printk(KERN_DEBUG"pdev = %p ", pdev);

Use pr_debug. Don't use __DATE__ and __TIME__, the person should know when they build their code.

samples/ltd/ldt.c line 564

Quoted 9 lines
samples/ltd/ldt.c new file +9 -0
+	pr_debug_dec(irq);
+	pr_debug_dec(bufsize);
+	in_buf = alloc_pages_exact(bufsize, GFP_KERNEL | __GFP_ZERO);
+	if (!in_buf) {
+		ret = -ENOMEM;
+		goto exit;
+	}
+	pages_flag(virt_to_page(in_buf), PFN_UP(bufsize), PG_reserved, 1);
+	out_buf = alloc_pages_exact(bufsize, GFP_KERNEL | __GFP_ZERO);

alloc_pages_exact is not used by very many drivers, and none of the uses appear to be for kfifos. This sets a potentially bad example for driver writers. Any reason this can't be kzalloc?

samples/ltd/ldt.c line 576

Quoted 12 lines
samples/ltd/ldt.c new file +12 -0
+	if (!out_buf) {
+		ret = -ENOMEM;
+		goto exit;
+	}
+	pages_flag(virt_to_page(out_buf), PFN_UP(bufsize), PG_reserved, 1);
+	if (pdev) {
+		dev_dbg(&pdev->dev, "%s:%d %s attaching driver\n", __file__, __LINE__, __func__);
+		pr_debug_hex(pdev->dev.of_node);
+#ifdef CONFIG_OF_DEVICE
+		check(of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev));
+#endif
+		data = pdev->dev.platform_data;

Use dev_get_platdata(&pdev->dev);

samples/ltd/ldt.c line 580

Quoted 4 lines
samples/ltd/ldt.c new file +4 -0
+		printk("%p %s\n", data, data);
+		if (!irq)
+			irq = platform_get_irq(pdev, 0);
+		r = platform_get_resource(pdev, IORESOURCE_IO, 0);

If this fails, shouldn't the probe fail?

samples/ltd/ldt.c line 588

Quoted 8 lines
samples/ltd/ldt.c new file +8 -0
+		if (r && !port)
+			port = r->start;
+
+		if (r && !port_size)
+			port_size = resource_size(r);
+	}
+	isr_counter = 0;
+	uart_probe();

uart_probe returns a value, which is not checked here?

samples/ltd/ldt.c line 594

Quoted 6 lines
samples/ltd/ldt.c new file +6 -0
+	/* proc_create(ldt_name, 0, NULL, &ldt_fops); depricated */
+	mod_timer(&ldt_timer, jiffies + HZ / 10);
+	thread = kthread_run(ldt_thread, NULL, "%s", ldt_name);
+	if (IS_ERR(thread)) {
+		ret = PTR_ERR(thread);
+		if (ret)

This if test will always be true.

samples/ltd/ldt.c line 597

Quoted 3 lines
samples/ltd/ldt.c new file +3 -0
+			goto exit;
+	}
+	debugfs = debugfs_create_file(ldt_name, S_IRUGO, NULL, NULL, &ldt_fops);

Error checking.

samples/ltd/ldt.c line 606

Quoted 9 lines
samples/ltd/ldt.c new file +9 -0
+#ifdef USE_MISCDEV
+	ret = check(misc_register(&ldt_miscdev));
+	if (ret < 0)
+		goto exit;
+	pr_debug_dec(ldt_miscdev.minor);
+#else
+	chrdev_region_init(ldt_name);
+#endif
+exit:

This does no cleanup if the probe partially succeeded.

samples/ltd/ldt.c line 618

Quoted 12 lines
samples/ltd/ldt.c new file +12 -0
+	pr_debug_dec(ret);
+	return ret;
+}
+
+/*
+ *	ldt_remove - main clean up function
+ */
+
+static int __devexit ldt_remove(struct platform_device *pdev)
+{
+	int d;
+	if (pdev)

pdev should always be valid.

Quoted 79 lines
samples/ltd/ldt.c new file +79 -0
+		dev_dbg(&pdev->dev, "%s:%d %s detaching driver\n", __file__, __LINE__, __func__);
+	/* remove_proc_entry(ldt_name, NULL); depricated */
+	if (debugfs)
+		debugfs_remove(debugfs);
+#ifdef USE_MISCDEV
+	misc_deregister(&ldt_miscdev);
+#else
+	for (d = 0; d < devs; d++)
+		device_destroy(ldt_class, MKDEV(major, d));
+	class_destroy(ldt_class);
+	cdev_del(&ldt_cdev);
+	unregister_chrdev_region(MKDEV(major, 0), devs);
+#endif
+	if (!IS_ERR_OR_NULL(thread)) {
+		send_sig(SIGINT, thread, 1);
+		kthread_stop(thread);
+	}
+	del_timer(&ldt_timer);
+	if (port_r)
+		release_region(port, port_size);
+	if (irq) {
+		if (uart_detected) {
+			iowrite8(0, port_ptr + UART_IER);
+			iowrite8(0, port_ptr + UART_FCR);
+			iowrite8(0, port_ptr + UART_MCR);
+			ioread8(port_ptr + UART_RX);
+		}
+		free_irq(irq, THIS_MODULE);
+	}
+	tasklet_kill(&ldt_tasklet);
+	if (in_buf) {
+		pages_flag(virt_to_page(in_buf), PFN_UP(bufsize), PG_reserved, 0);
+		free_pages_exact(in_buf, bufsize);
+	}
+	if (out_buf) {
+		pages_flag(virt_to_page(out_buf), PFN_UP(bufsize), PG_reserved, 0);
+		free_pages_exact(out_buf, bufsize);
+	}
+	pr_debug_dec(isr_counter);
+	pr_debug_dec(ldt_work_counter);
+	if (port_ptr)
+		ioport_unmap(port_ptr);
+	return 0;
+}
+
+#ifdef USE_PLATFORM_DEVICE
+
+/*
+ * Following code requires platform_device (ldt_plat_dev.*) to work
+ */
+
+#ifdef CONFIG_PM
+
+static int ldt_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int ldt_resume(struct device *dev)
+{
+	return 0;
+}
+
+static const struct dev_pm_ops ldt_pm = {
+	.suspend = ldt_suspend,
+	.resume = ldt_resume,
+};
+
+#define ldt_pm_ops (&ldt_pm)
+#else
+#define ldt_pm_ops NULL
+#endif
+
+static const struct of_device_id ldt_of_match[] = {
+	{.compatible = "linux-driver-template",},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, ldt_of_match);

samples/ltd/ldt.c line 707

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+
+static struct platform_driver ldt_driver = {
+	.driver = {
+		   .name = "ldt_device_name",
+		   .owner = THIS_MODULE,
+		   .pm = ldt_pm_ops,
+		   .of_match_table = of_match_ptr(ldt_of_match),
+		   },
+	.probe = ldt_probe,
+	.remove = __devexit_p(ldt_remove),

Tabify.

samples/ltd/ldt.c line 712

Quoted 5 lines
samples/ltd/ldt.c new file +5 -0
+};
+
+#ifdef module_platform_driver
+module_platform_driver(ldt_driver);
+#else

Why would this not be defined?

samples/ltd/ldt.c line 717

Quoted 5 lines
samples/ltd/ldt.c new file +5 -0
+
+/*
+ *	for Linux kernel releases before v3.1-12
+ *	without macro module_platform_driver
+ */

If you are aiming for merging to mainline the kernel version stuff becomes irrelevant.

samples/ltd/ldt.c line 723

Quoted 6 lines
samples/ltd/ldt.c new file +6 -0
+
+static int ldt_init(void)
+{
+	int ret = 0;
+	ret = platform_driver_register(&ldt_driver);
+	return ret;
	return platform_driver_register(&ldt_driver);
Quoted 16 lines
samples/ltd/ldt.c new file +16 -0
+}
+
+static void ldt_exit(void)
+{
+	platform_driver_unregister(&ldt_driver);
+}
+
+module_init(ldt_init);
+module_exit(ldt_exit);
+#endif /* module_platform_driver */
+
+#else /* !USE_PLATFORM_DEVICE */
+
+/*
+ *	Standalone module initialization to run without platform_device
+ */

samples/ltd/ldt.c line 749

Quoted 10 lines
samples/ltd/ldt.c new file +10 -0
+
+static int ldt_init(void)
+{
+	int ret = 0;
+	/*
+	 *      Call probe function directly,
+	 *      bypassing platform_device infrastructure
+	 */
+	ret = ldt_probe(NULL);
+	return ret;
	return ldt_probe(NULL);

samples/ltd/ldt.c line 755

Quoted 6 lines
samples/ltd/ldt.c new file +6 -0
+}
+
+static void ldt_exit(void)
+{
+	int res;
+	res = ldt_remove(NULL);

What is the point of the res variable?

Quoted 41 lines
samples/ltd/ldt.c new file +9 -0
+}
+
+module_init(ldt_init);
+module_exit(ldt_exit);
+#endif
+
+MODULE_DESCRIPTION("LDT - Linux Driver Template");
+MODULE_AUTHOR("Constantine Shulyupin <[email protected]>");
+MODULE_LICENSE("Dual BSD/GPL");
samples/ltd/ldt_plat_dev.c new file +26 -0
new file mode 100644
index 0000000..cd45057
@@ -0,0 +1,68 @@
+/*
+ *	LDT - Linux Driver Template
+ *
+ *	Copyright (C) 2012 Constantine Shulyupin  http://www.makelinux.net/
+ *
+ *	Dual BSD/GPL License
+ *
+ *	platform_device template driver
+ *
+ *	uses
+ *
+ *	platform_data
+ *	resources
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include "tracing.h"
+
+static struct resource ldt_resource[] = {
+	{
+	 .flags = IORESOURCE_IO,
+	 .start = 0x3f8,
+	 .end = 0x3ff,
+	 },

samples/ltd/ldt_plat_dev.c line 36

Quoted 10 lines
samples/ltd/ldt_plat_dev.c new file +10 -0
+	{
+	 .flags = IORESOURCE_IRQ,
+	 .start = 4,
+	 .end = 4,
+	 },
+	{
+	 .flags = IORESOURCE_MEM,
+	 .start = 0,
+	 .end = 0,
+	 },

Identation is all messed here.

samples/ltd/ldt_plat_dev.c line 42

Quoted 6 lines
samples/ltd/ldt_plat_dev.c new file +6 -0
+};
+
+void ldt_dev_release(struct device *dev)
+{
+_entry:;
+}

Just remove the body.

samples/ltd/ldt_plat_dev.c line 48

Quoted 5 lines
samples/ltd/ldt_plat_dev.c new file +5 -0
+static struct platform_device ldt_platform_device = {
+	.name = "ldt_device_name",
+	.id = 0,
+	.num_resources = ARRAY_SIZE(ldt_resource),
+	.resource = ldt_resource,

Typically put resource first, then num_resources.

Quoted 10 lines
samples/ltd/ldt_plat_dev.c new file +10 -0
+	.dev.platform_data = "test data",
+	.dev.release = ldt_dev_release,
+};
+
+static int ldt_plat_dev_init(void)
+{
+	return platform_device_register(&ldt_platform_device);
+}
+
+static void ldt_plat_dev_exit(void)

samples/ltd/ldt_plat_dev.c line 68

Quoted 10 lines
samples/ltd/ldt_plat_dev.c new file +10 -0
+{
+	platform_device_unregister(&ldt_platform_device);
+}
+
+module_init(ldt_plat_dev_init);
+module_exit(ldt_plat_dev_exit);
+
+MODULE_DESCRIPTION("LDT - Linux Driver Template: platform_device");
+MODULE_AUTHOR("Constantine Shulyupin <[email protected]>");
+MODULE_LICENSE("Dual BSD/GPL");

~Ryan

#12 lore raw
In reply to Constantine Shulyupin

Doing this is the past has always led to obsolete stale broken examples because they are not actively in use and maintained. Far better to work from actual live in use driver code.

Alan

#11 lore raw
In reply to Ryan Mallon
Quoted 5 lines · Ryan Mallon
Quoted 2 lines · Constantine Shulyupin

From: Constantine Shulyupin <[email protected]>
+ if (kfifo_is_empty(&in_fifo)) {

Doesn't this require locking against whatever is filling the fifo?

I seems doesn't.
Note from include/linux/kfifo.h
/*
* Note about locking : There is no locking required until only * one reader
* and one writer is using the fifo and no kfifo_reset() will be * called
* kfifo_reset_out() can be safely used, until it will be only called
* in the reader thread.
* For multiple writer and one reader there is only a need to lock the writer.
* And vice versa for only one writer and multiple reader there is only a need
* to lock the reader.
*/

Quoted 4 lines
Quoted 1 line

+ if (mutex_lock_interruptible(&read_lock)) {

The read_lock is only used here, so would only protect against concurrent reads. Isn't the read for a device function already synchronised against itself by the caller?

I can't find locking for read/write in sys_read, vfs_read I just found lock misc_mtx for misc_open, misc_register.

Quoted 4 lines
Quoted 1 line

+ ret = kfifo_from_user(&out_fifo, buf, count, &copied);

Shouldn't this function be grabbing fifo_lock to prevent concurrent access to the fifo by the tasklet function? write_lock has the same issues described for read_lock above.

Accordingly note about kfifo locking is shouldn't. kfifo supports asynchronous i/o.

Thank you.

#13 lore raw
In reply to Alan Cox
Quoted 3 lines · Alan Cox

Doing this is the past has always led to obsolete stale broken examples because they are not actively in use and maintained. Far better to work from actual live in use driver code.

exactly what I tell him on ce-linux

Best Regards,
J.