[PATCH v2] LDT - Linux Driver Template


Constantine Shulyupin
 

From: Constantine Shulyupin <const@...>

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, single misc device,
Device Model, configfs, UART 0x3f8,
HW loopback, SW loopback, ftracer.

Thanks for review and valuable comments to Greg Kroah-Hartman and Ryan Mallon

Signed-off-by: Constantine Shulyupin <const@...>

---

Changelog

Changes since v1
- removed tracing macros and replaced with pr_debug
- removed multiple char devices (alloc_chrdev_region, class_create) and used only misc device
- added module parameters descriptions
- fixed braces
- wrapped long lines
- fixed declaration and initialization of some variables
- improved comments
- reworked some functions
- added blocking write
- edited log messages
- added more error checking
- added tabulation of structs
- removed standalone module_init and module_exit in favour module_platform_driver
- added driver data struct

fix
---
samples/Kconfig | 2 +
samples/Makefile | 5 +-
samples/ldt/Kconfig | 14 +
samples/ldt/Makefile | 1 +
samples/ldt/ldt.c | 716 ++++++++++++++++++++++++++++++++++++++++++++
samples/ldt/ldt_plat_dev.c | 65 ++++
scripts/checkpatch.pl | 4 +-
tools/testing/ldt/Makefile | 4 +
tools/testing/ldt/dio.c | 319 ++++++++++++++++++++
tools/testing/ldt/ldt-test | 142 +++++++++
10 files changed, 1268 insertions(+), 4 deletions(-)
create mode 100644 samples/ldt/Kconfig
create mode 100644 samples/ldt/Makefile
create mode 100644 samples/ldt/ldt.c
create mode 100644 samples/ldt/ldt_plat_dev.c
create mode 100644 tools/testing/ldt/Makefile
create mode 100644 tools/testing/ldt/dio.c
create mode 100755 tools/testing/ldt/ldt-test

diff --git a/samples/Kconfig b/samples/Kconfig
index 7b6792a..1493bdb 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -69,4 +69,6 @@ config SAMPLE_RPMSG_CLIENT
to communicate with an AMP-configured remote processor over
the rpmsg bus.

+source "samples/ldt/Kconfig"
+
endif # SAMPLES
diff --git a/samples/Makefile b/samples/Makefile
index 5ef08bb..d4b1818 100644
--- a/samples/Makefile
+++ b/samples/Makefile
@@ -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/
diff --git a/samples/ldt/Kconfig b/samples/ldt/Kconfig
new file mode 100644
index 0000000..bd0251c
--- /dev/null
+++ b/samples/ldt/Kconfig
@@ -0,0 +1,14 @@
+config SAMPLE_DRIVER_TEMPLATE
+ tristate "LDT - Linux driver template"
+ depends on m
+ 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
diff --git a/samples/ldt/Makefile b/samples/ldt/Makefile
new file mode 100644
index 0000000..14ceea1
--- /dev/null
+++ b/samples/ldt/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SAMPLE_DRIVER_TEMPLATE)+= ldt.o ldt_plat_dev.o
diff --git a/samples/ldt/ldt.c b/samples/ldt/ldt.c
new file mode 100644
index 0000000..5bfc2d0
--- /dev/null
+++ b/samples/ldt/ldt.c
@@ -0,0 +1,716 @@
+/*
+ * LDT - Linux Driver Template
+ *
+ * Copyright (C) 2012 Constantine Shulyupin http://www.makelinux.net/
+ *
+ * GPL License
+ *
+ *
+ * The driver demonstrates usage of following Linux facilities:
+ *
+ * Linux kernel module
+ * file_operations
+ * read and write (UART)
+ * blocking read and write
+ * 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
+ * Power Management (dev_pm_ops)
+ * Device Tree (of_device_id)
+ *
+ * Usermode test script and utility are located in tools/testing/ldt/
+ *
+
+ */
+
+#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/slab.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>
+
+static int port;
+module_param(port, int, 0);
+MODULE_PARM_DESC(port, "io port number, default 0x3f8 - UART");
+
+static int port_size;
+module_param(port_size, int, 0);
+MODULE_PARM_DESC(port_size, "number of io ports, default 8");
+
+static int irq;
+module_param(irq, int, 0);
+MODULE_PARM_DESC(irq, "interrupt request number, default 4 - UART");
+
+static int loopback;
+module_param(loopback, int, 0);
+MODULE_PARM_DESC(loopback, "loopback mode for testing, default 0");
+
+#define FIFO_SIZE 128 /* must be power of two */
+
+static int bufsize = 8 * PAGE_SIZE;
+static int uart_detected;
+static void __iomem *port_ptr;
+
+/*
+ * ldt_data - the driver data
+ * stored in static global variable for simplicity.
+ * Can be also retrieved from platform_device with
+ * struct ldt_data *drvdata = platform_get_drvdata(pdev);
+ */
+
+struct ldt_data {
+ void *in_buf;
+ void *out_buf;
+ DECLARE_KFIFO(in_fifo, char, FIFO_SIZE);
+ DECLARE_KFIFO(out_fifo, char, FIFO_SIZE);
+ spinlock_t fifo_lock;
+ wait_queue_head_t readable, writeable;
+};
+
+static struct ldt_data *drvdata;
+
+/*
+ * ldt_received
+ * Called from tasklet, which is fired from ISR or timer,
+ * with data received from HW port
+ */
+
+static void ldt_received(char data)
+{
+ kfifo_in_spinlocked(&drvdata->in_fifo, &data,
+ sizeof(data), &drvdata->fifo_lock);
+ wake_up_interruptible(&drvdata->readable);
+}
+
+/*
+ * ldt_send - send data to HW port or emulated SW loopback
+ */
+
+static void ldt_send(char data)
+{
+ if (uart_detected)
+ iowrite8(data, port_ptr + UART_TX);
+ else
+ if (loopback)
+ ldt_received(data);
+}
+
+static inline u8 tx_ready(void)
+{
+ return ioread8(port_ptr + UART_LSR) & UART_LSR_THRE;
+}
+
+static inline u8 rx_ready(void)
+{
+ return ioread8(port_ptr + UART_LSR) & UART_LSR_DR;
+}
+
+/*
+ * work section
+ *
+ * empty template function for deferred call in scheduler context
+ */
+
+static int ldt_work_counter;
+
+
+static void ldt_work_func(struct work_struct *work)
+{
+ ldt_work_counter++;
+}
+
+static DECLARE_WORK(ldt_work, ldt_work_func);
+
+/*
+ * tasklet section
+ *
+ * template function for deferred call in interrupt context
+ */
+
+static DECLARE_COMPLETION(ldt_complete);
+
+static void ldt_tasklet_func(unsigned long d)
+{
+ char data_out, data_in;
+
+ if (uart_detected) {
+ while (tx_ready() && kfifo_out_spinlocked(&drvdata->out_fifo,
+ &data_out, sizeof(data_out), &drvdata->fifo_lock)) {
+ wake_up_interruptible(&drvdata->writeable);
+ pr_debug("UART_LSR=0x%02X\n", ioread8(port_ptr + UART_LSR));
+ pr_debug("%s: data_out=%d %c\n", __func__, data_out, data_out >= 32 ? data_out : 0);
+ ldt_send(data_out);
+ }
+ while (rx_ready()) {
+ pr_debug("UART_LSR=0x%02X\n", ioread8(port_ptr + UART_LSR));
+ data_in = ioread8(port_ptr + UART_RX);
+ pr_debug("%s: data_in=%d %c\n", __func__, data_in, data_in >= 32 ? data_in : 0);
+ ldt_received(data_in);
+ }
+ } else {
+ while (kfifo_out_spinlocked(&drvdata->out_fifo, &data_out, sizeof(data_out),
+ &drvdata->fifo_lock)) {
+ wake_up_interruptible(&drvdata->writeable);
+ pr_debug("%s: data_out=%d\n", __func__, data_out);
+ ldt_send(data_out);
+ }
+ }
+ schedule_work(&ldt_work);
+ complete(&ldt_complete);
+}
+
+static DECLARE_TASKLET(ldt_tasklet, ldt_tasklet_func, 0);
+
+/*
+ * interrupt section
+ */
+
+static int isr_counter;
+
+static irqreturn_t ldt_isr(int irq, void *dev_id)
+{
+ /*
+ * UART interrupt is not fired in loopback mode,
+ * therefore fire ldt_tasklet from timer too
+ */
+ isr_counter++;
+ pr_debug("UART_FCR=0x%02X\n", ioread8(port_ptr + UART_FCR));
+ pr_debug("UART_IIR=0x%02X\n", ioread8(port_ptr + UART_IIR));
+ tasklet_schedule(&ldt_tasklet);
+ return IRQ_HANDLED; /* our IRQ */
+}
+
+/*
+ * timer section
+ */
+
+static struct timer_list ldt_timer;
+
+static void ldt_timer_func(unsigned long data)
+{
+ /*
+ * this timer is used just to fire ldt_tasklet,
+ * because there is no interrupts 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 section
+ */
+
+static int ldt_open(struct inode *inode, struct file *file)
+{
+ pr_debug("%s: from %s\n", __func__,current->comm);
+ pr_debug("imajor=%d iminor=%d \n", imajor(inode), iminor(inode));
+ pr_debug("f_flags & O_NONBLOCK=0x%X\n", file->f_flags & O_NONBLOCK);
+ /* client related data can be allocated here and
+ stored in file->private_data */
+ return 0;
+}
+
+static int ldt_release(struct inode *inode, struct file *file)
+{
+ pr_debug("%s: from %s\n", __func__,current->comm);
+ pr_debug("imajor=%d iminor=%d \n", imajor(inode), iminor(inode));
+ pr_debug("isr_counter=%d\n", isr_counter);
+ pr_debug("ldt_work_counter=%d\n", ldt_work_counter);
+ /* client related data can be retrived from file->private_data
+ and released here */
+ return 0;
+}
+
+static DEFINE_MUTEX(read_lock);
+
+static ssize_t ldt_read(struct file *file, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ int ret = 0;
+ unsigned int copied;
+
+ pr_debug("%s: from %s\n", __func__,current->comm);
+ if (kfifo_is_empty(&drvdata->in_fifo)) {
+ if (file->f_flags & O_NONBLOCK) {
+ return -EAGAIN;
+ } else {
+ pr_debug("%s: %s\n", __func__, "waiting");
+ ret = wait_event_interruptible(drvdata->readable,
+ !kfifo_is_empty(&drvdata->in_fifo));
+ if (ret == -ERESTARTSYS) {
+ pr_err("%s:%d %s %s\n", __FILE__, __LINE__, __func__, "interrupted");
+ return -EINTR;
+ }
+ }
+ }
+ if (mutex_lock_interruptible(&read_lock))
+ return -EINTR;
+ ret = kfifo_to_user(&drvdata->in_fifo, buf, count, &copied);
+ mutex_unlock(&read_lock);
+ return ret ? ret : copied;
+}
+
+static DEFINE_MUTEX(write_lock);
+
+static ssize_t ldt_write(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ int ret;
+ unsigned int copied;
+
+ pr_debug("%s: from %s\n", __func__,current->comm);
+ if (kfifo_is_full(&drvdata->out_fifo)) {
+ if (file->f_flags & O_NONBLOCK) {
+ return -EAGAIN;
+ } else {
+ ret = wait_event_interruptible(drvdata->writeable,
+ !kfifo_is_full(&drvdata->out_fifo));
+ if (ret == -ERESTARTSYS) {
+ pr_err("%s:%d %s %s\n", __FILE__, __LINE__, __func__, "interrupted");
+ return -EINTR;
+ }
+ }
+ }
+ if (mutex_lock_interruptible(&write_lock))
+ return -EINTR;
+ ret = kfifo_from_user(&drvdata->out_fifo, buf, count, &copied);
+ mutex_unlock(&write_lock);
+ tasklet_schedule(&ldt_tasklet);
+ return ret ? ret : copied;
+}
+
+static unsigned int ldt_poll(struct file *file, poll_table *pt)
+{
+ unsigned int mask = 0;
+ poll_wait(file, &drvdata->readable, pt);
+ poll_wait(file, &drvdata->writeable, pt);
+
+ if (!kfifo_is_empty(&drvdata->in_fifo))
+ mask |= POLLIN | POLLRDNORM;
+ mask |= POLLOUT | POLLWRNORM;
+/*
+ if case of output end of file set
+ mask |= POLLHUP;
+ in case of output error set
+ mask |= POLLERR;
+*/
+ return mask;
+}
+
+/*
+ * pages_flag - set or clear a flag for sequence of pages
+ *
+ * more generic solution instead SetPageReserved, ClearPageReserved etc
+ *
+ * Poposing to move pages_flag to linux/page-flags.h
+ */
+
+static void pages_flag(struct page *page, int page_num, int mask, int value)
+{
+ for (; page_num; page_num--, page++)
+ if (value)
+ __set_bit(mask, &page->flags);
+ else
+ __clear_bit(mask, &page->flags);
+}
+
+static int ldt_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+ void *buf = NULL;
+ if (vma->vm_flags & VM_WRITE)
+ buf = drvdata->in_buf;
+ else if (vma->vm_flags & VM_READ)
+ buf = drvdata->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("%s:%d %s %s\n", __FILE__, __LINE__, __func__, "remap_pfn_range failed");
+ return -EAGAIN;
+ }
+ return 0;
+}
+
+#define trace_ioctl(nr) pr_debug("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 DEFINE_MUTEX(ioctl_lock);
+
+static long ldt_ioctl(struct file *f, unsigned int cmnd, unsigned long arg)
+{
+ int ret = 0;
+ void __user *user = (void __user *)arg;
+
+ if (mutex_lock_interruptible(&ioctl_lock))
+ return -EINTR;
+ pr_debug("%s:\n", __func__);
+ pr_debug("cmnd=0x%X\n", cmnd);
+ pr_debug("arg=0x%lX\n", arg);
+ trace_ioctl(cmnd);
+ switch (_IOC_TYPE(cmnd)) {
+ case 'A':
+ switch (_IOC_NR(cmnd)) {
+ case 0:
+ if (_IOC_DIR(cmnd) == _IOC_WRITE) {
+ if (copy_from_user(drvdata->in_buf, user,
+ _IOC_SIZE(cmnd))) {
+ ret = -EFAULT;
+ goto exit;
+ }
+ /* copy data from in_buf to out_buf to emulate loopback for testing */
+ memcpy(drvdata->out_buf, drvdata->in_buf, bufsize);
+ memset(drvdata->in_buf, 0, bufsize);
+ }
+ if (_IOC_DIR(cmnd) == _IOC_READ) {
+ if (copy_to_user(user, drvdata->out_buf,
+ _IOC_SIZE(cmnd))) {
+ ret = -EFAULT;
+ goto exit;
+ }
+ memset(drvdata->out_buf, 0, bufsize);
+ }
+ break;
+ }
+ break;
+ }
+exit:
+ mutex_unlock(&ioctl_lock);
+ return ret;
+}
+
+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,
+};
+
+static struct miscdevice ldt_miscdev = {
+ .minor = MISC_DYNAMIC_MINOR,
+ .name = KBUILD_MODNAME,
+ .fops = &ldt_fops,
+};
+
+/*
+ * kthread section
+ */
+
+static int ldt_thread(void *data)
+{
+ int ret = 0;
+ allow_signal(SIGINT);
+ while (!kthread_should_stop()) {
+ ret = wait_for_completion_interruptible(&ldt_complete);
+ if (ret == -ERESTARTSYS) {
+ pr_debug("%s: %s\n", __func__, "interrupted");
+ return -EINTR;
+ }
+ /*
+ perform here a useful work in scheduler context
+ */
+ }
+ return ret;
+}
+
+/*
+ * UART initialization section
+ */
+
+static struct resource *port_r;
+
+static int uart_probe(void)
+{
+ int ret = 0;
+
+ if (port) {
+ port_r = request_region(port, port_size, KBUILD_MODNAME);
+ if (!port_r) {
+ pr_err("%s:%d %s %s\n", __FILE__, __LINE__, __func__, "request_region failed");
+ return -EBUSY;
+ }
+ port_ptr = ioport_map(port, port_size);
+ pr_debug("%s: port_ptr=%p\n", __func__, port_ptr);
+ if (!port_ptr) {
+ pr_err("%s:%d %s %s\n", __FILE__, __LINE__, __func__, "ioport_map failed");
+ return -ENODEV;
+ }
+ }
+ if (irq && port_ptr) {
+ /*
+ * Minimal configuration of UART for trivial I/O opertaions
+ * and ISR just to porform basic tests.
+ * Some configuration of UART is not touched and reused.
+ *
+ * This minimal configiration of UART is based on
+ * full UART driver drivers/tty/serial/8250/8250.c
+ */
+ ret = request_irq(irq, ldt_isr,
+ IRQF_SHARED, KBUILD_MODNAME, THIS_MODULE);
+ if (ret < 0) {
+ pr_err("%s:%d %s %s\n", __FILE__, __LINE__, __func__, "request_irq failed");
+ return ret;
+ }
+ 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("UART_MSR=0x%02X\n", ioread8(port_ptr + UART_MSR));
+ pr_debug("%s: uart_detected=0x%X\n", __func__, uart_detected);
+
+ if (uart_detected) {
+ 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("%s: loopback=%d\n", __func__, loopback);
+ if (loopback)
+ iowrite8(ioread8(port_ptr + UART_MCR) | UART_MCR_LOOP,
+ port_ptr + UART_MCR);
+ }
+ if (!uart_detected && loopback)
+ pr_warn("Emulating loopback in software\n");
+ }
+ return ret;
+}
+
+/*
+ * main initialization and cleanup section
+ */
+
+static struct task_struct *thread;
+static struct dentry *debugfs;
+
+static int ldt_cleanup(struct platform_device *pdev)
+{
+ struct ldt_data *drvdata = platform_get_drvdata(pdev);
+ dev_dbg(&pdev->dev, "%s\n", __func__);
+
+ if (!IS_ERR_OR_NULL(debugfs))
+ debugfs_remove(debugfs);
+ if (ldt_miscdev.this_device)
+ misc_deregister(&ldt_miscdev);
+ if (!IS_ERR_OR_NULL(thread)) {
+ send_sig(SIGINT, thread, 1);
+ kthread_stop(thread);
+ }
+ del_timer(&ldt_timer);
+ 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 (drvdata->in_buf) {
+ pages_flag(virt_to_page(drvdata->in_buf), PFN_UP(bufsize), PG_reserved, 0);
+ free_pages_exact(drvdata->in_buf, bufsize);
+ }
+ if (drvdata->out_buf) {
+ pages_flag(virt_to_page(drvdata->out_buf), PFN_UP(bufsize), PG_reserved, 0);
+ free_pages_exact(drvdata->out_buf, bufsize);
+ }
+
+ pr_debug("%s: isr_counter=%d\n", __func__, isr_counter);
+ pr_debug("%s: ldt_work_counter=%d\n", __func__, ldt_work_counter);
+ if (port_ptr)
+ ioport_unmap(port_ptr);
+ if (port_r)
+ release_region(port, port_size);
+ kfree(drvdata);
+ platform_set_drvdata(pdev, NULL);
+ return 0;
+}
+
+int ldt_data_init(struct platform_device *pdev)
+{
+ drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
+ if (!drvdata)
+ return -ENOMEM;
+ init_waitqueue_head(&drvdata->readable);
+ init_waitqueue_head(&drvdata->writeable);
+ INIT_KFIFO(drvdata->in_fifo);
+ INIT_KFIFO(drvdata->out_fifo);
+ platform_set_drvdata(pdev, drvdata);
+ return 0;
+}
+
+static __devinit int ldt_probe(struct platform_device *pdev)
+{
+ int ret;
+ char *data = NULL;
+ struct resource *r;
+ dev_dbg(&pdev->dev, "%s attaching device\n", __func__);
+ pr_debug("MODNAME=%s\n", KBUILD_MODNAME);
+ pr_debug("port = %d irq = %d\n", port, irq);
+
+ ret = ldt_data_init(pdev);
+ if (ret < 0) {
+ pr_err("%s:%d %s %s\n", __FILE__, __LINE__, __func__, "ldt_data_init failed");
+ goto exit;
+ }
+
+ /*
+ * Allocating buffers and pinning them to RAM
+ * to be mapped to user space in ldt_mmap
+ */
+ drvdata->in_buf = alloc_pages_exact(bufsize, GFP_KERNEL | __GFP_ZERO);
+ if (!drvdata->in_buf) {
+ ret = -ENOMEM;
+ goto exit;
+ }
+ pages_flag(virt_to_page(drvdata->in_buf), PFN_UP(bufsize), PG_reserved, 1);
+ drvdata->out_buf = alloc_pages_exact(bufsize, GFP_KERNEL | __GFP_ZERO);
+ if (!drvdata->out_buf) {
+ ret = -ENOMEM;
+ goto exit;
+ }
+ pages_flag(virt_to_page(drvdata->out_buf), PFN_UP(bufsize), PG_reserved, 1);
+ pr_debug("%s: pdev->dev.of_node=%p\n", __func__, pdev->dev.of_node);
+#ifdef CONFIG_OF_DEVICE
+ of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+#endif
+ data = dev_get_platdata(&pdev->dev);
+ pr_debug("%p %s\n", data, data);
+ if (!irq)
+ irq = platform_get_irq(pdev, 0);
+ r = platform_get_resource(pdev, IORESOURCE_IO, 0);
+ if (r && !port)
+ port = r->start;
+
+ if (r && !port_size)
+ port_size = resource_size(r);
+ isr_counter = 0;
+ /*
+ * This drivers without UART can be sill used
+ * in emulation mode for testing and demonstation of work
+ */
+ ret = uart_probe();
+ if (ret < 0) {
+ pr_err("%s:%d %s %s\n", __FILE__, __LINE__, __func__, "uart_probe failed");
+ goto exit;
+ }
+ mod_timer(&ldt_timer, jiffies + HZ / 10);
+ thread = kthread_run(ldt_thread, NULL, "%s", KBUILD_MODNAME);
+ if (IS_ERR(thread)) {
+ ret = PTR_ERR(thread);
+ goto exit;
+ }
+ debugfs = debugfs_create_file(KBUILD_MODNAME, S_IRUGO, NULL, NULL, &ldt_fops);
+ if (IS_ERR(debugfs)) {
+ ret = PTR_ERR(debugfs);
+ pr_err("%s:%d %s %s\n", __FILE__, __LINE__, __func__, "debugfs_create_file failed");
+ goto exit;
+ }
+ ret = misc_register(&ldt_miscdev);
+ if (ret < 0) {
+ pr_err("%s:%d %s %s\n", __FILE__, __LINE__, __func__, "misc_register failed");
+ goto exit;
+ }
+ pr_debug("%s: ldt_miscdev.minor=%d\n", __func__, ldt_miscdev.minor);
+
+exit:
+ pr_debug("%s: ret=%d\n", __func__, ret);
+ if (ret < 0)
+ ldt_cleanup(pdev);
+ return ret;
+}
+
+static int __devexit ldt_remove(struct platform_device *pdev)
+{
+ dev_dbg(&pdev->dev, "%s detaching device\n", __func__);
+ ldt_cleanup(pdev);
+ return 0;
+}
+
+/*
+ * 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
+
+/*
+ * template for OF FDT ID
+ * (Open Firmware Flat Device Tree)
+ */
+
+static const struct of_device_id ldt_of_match[] = {
+ {.compatible = "linux-driver-template",},
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, ldt_of_match);
+
+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),
+};
+
+module_platform_driver(ldt_driver);
+
+MODULE_DESCRIPTION("LDT - Linux Driver Template");
+MODULE_AUTHOR("Constantine Shulyupin <const@...>");
+MODULE_LICENSE("GPL");
diff --git a/samples/ldt/ldt_plat_dev.c b/samples/ldt/ldt_plat_dev.c
new file mode 100644
index 0000000..3d20a1a
--- /dev/null
+++ b/samples/ldt/ldt_plat_dev.c
@@ -0,0 +1,65 @@
+/*
+ * LDT - Linux Driver Template
+ *
+ * Copyright (C) 2012 Constantine Shulyupin http://www.makelinux.net/
+ *
+ * GPL License
+ *
+ * platform_device template driver
+ *
+ * uses
+ *
+ * platform_data
+ * resources
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+static struct resource ldt_resource[] = {
+ {
+ .flags = IORESOURCE_IO,
+ .start = 0x3f8,
+ .end = 0x3ff,
+ },
+ {
+ .flags = IORESOURCE_IRQ,
+ .start = 4,
+ .end = 4,
+ },
+ {
+ .flags = IORESOURCE_MEM,
+ .start = 0,
+ .end = 0,
+ },
+};
+
+void ldt_dev_release(struct device *dev)
+{
+}
+
+static struct platform_device ldt_platform_device = {
+ .name = "ldt_device_name",
+ .resource = ldt_resource,
+ .num_resources = ARRAY_SIZE(ldt_resource),
+ .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)
+{
+ 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 <const@...>");
+MODULE_LICENSE("GPL");
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f18750e..a9d9914 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1779,10 +1779,10 @@ sub process {
$rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
!($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ ||
$line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
- $length > 80)
+ $length > 90)
{
WARN("LONG_LINE",
- "line over 80 characters\n" . $herecurr);
+ "line ".$length." over 90 characters\n" . $herecurr);
}

# Check for user-visible strings broken across lines, which breaks the ability
diff --git a/tools/testing/ldt/Makefile b/tools/testing/ldt/Makefile
new file mode 100644
index 0000000..cfa4ac8
--- /dev/null
+++ b/tools/testing/ldt/Makefile
@@ -0,0 +1,4 @@
+all: dio
+
+clean:
+ rm -rf dio *.o dio *.tmp *.log
diff --git a/tools/testing/ldt/dio.c b/tools/testing/ldt/dio.c
new file mode 100644
index 0000000..b974bc5
--- /dev/null
+++ b/tools/testing/ldt/dio.c
@@ -0,0 +1,319 @@
+/*
+ * DIO - Device Input/Output utility for testing device drivers
+ *
+ * stdin/stdout <--> dio <--> mmap, ioctl, read/write
+ *
+ * Copyright (C) 2012 Constantine Shulyupin <const@...>
+ * http://www.makelinux.net/
+ *
+ * Dual BSD/GPL License
+ *
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <string.h>
+#include <errno.h>
+#include <getopt.h>
+#include <string.h>
+#include <sys/param.h>
+#include <sys/poll.h>
+#include <sys/wait.h>
+#include <sys/types.h>
+#include <sys/time.h>
+#include <sys/stat.h>
+#include <sys/mman.h>
+#include <sys/user.h>
+#include <time.h>
+#include <fcntl.h>
+#include <assert.h>
+#include <linux/ioctl.h>
+
+static enum io_type {
+ file_io,
+ mmap_io,
+ ioctl_io
+} io_type;
+
+static void *inbuf, *outbuf;
+static void *mm;
+static void *mem;
+static int buf_size;
+static int offset;
+static char *dev_name;
+static int ignore_eof;
+static int ioctl_num;
+static int loops;
+static int delay;
+static char ioctl_type = 'A';
+__thread int ret;
+static int ro, wo; /* read only, write only*/
+
+#define trace_dec(d) printf("%s = %ld ", #d, (long int)d)
+
+int output(int dev, void *buf, int size)
+{
+ ret = 0;
+ if (dev < 0 || ro)
+ return 0;
+ switch (io_type) {
+ case mmap_io:
+ memcpy(mem, buf, size);
+ ret = size;
+ break;
+ case ioctl_io:
+ ioctl(dev, _IOC(_IOC_WRITE, ioctl_type, ioctl_num, size & _IOC_SIZEMASK), buf);
+ break;
+ case file_io:
+ default:
+ ret = write(dev, buf, size);
+ }
+ return ret;
+}
+
+int input(int dev, void *buf, int size)
+{
+ ret = 0;
+ if (dev < 0 || wo)
+ return 0;
+ switch (io_type) {
+ case mmap_io:
+ memcpy(buf, mem, size);
+ ret = size;
+ break;
+ case ioctl_io:
+ ioctl(dev, _IOC(_IOC_READ, ioctl_type, ioctl_num, size & _IOC_SIZEMASK), buf);
+ ret = size;
+ break;
+ case file_io:
+ default:
+ ret = read(dev, buf, size);
+ }
+ return ret;
+}
+
+int io_start(int dev)
+{
+ struct pollfd pfd[2];
+ ssize_t data_in_len, data_out_len, len_total = 0;
+ int i = 0;
+
+ /* TODO: wo, ro */
+ pfd[0].fd = fileno(stdin);
+ pfd[0].events = POLLIN;
+ pfd[1].fd = dev;
+ pfd[1].events = POLLIN;
+ while (poll(pfd, sizeof(pfd) / sizeof(pfd[0]), -1) > 0) {
+ data_in_len = 0;
+ if (pfd[0].revents & POLLIN) {
+ pfd[0].revents = 0;
+ ret = data_in_len = read(fileno(stdin), inbuf, buf_size);
+ if (data_in_len < 0) {
+ usleep(100000);
+ break;
+ }
+ if (!data_in_len && !ignore_eof) {
+ /* read returns 0 on End Of File */
+ break;
+ }
+again:
+ ret = output(dev, inbuf, data_in_len);
+ if (ret < 0 && errno == EAGAIN) {
+ usleep(100000);
+ goto again;
+ }
+ if (data_in_len > 0)
+ len_total += data_in_len;
+ }
+ data_out_len = 0;
+ if (pfd[1].revents & POLLIN) {
+ pfd[1].revents = 0;
+ ret = data_out_len = input(dev, outbuf, buf_size);
+ if (data_out_len < 0) {
+ usleep(100000);
+ break;
+ }
+ if (!data_out_len) {
+ /* EOF, don't expect data from the file any more
+ but wee can continue to write */
+ pfd[1].events = 0;
+ }
+ if (!data_out_len && !ignore_eof) {
+ /* read returns 0 on End Of File */
+ break;
+ }
+ write(fileno(stdout), outbuf, data_out_len);
+ if (data_out_len > 0)
+ len_total += data_out_len;
+ }
+ if ((!ignore_eof && pfd[0].revents & POLLHUP) || pfd[1].revents & POLLHUP)
+ break;
+ i++;
+ if (loops && i >= loops)
+ break;
+ usleep(1000 * delay);
+ }
+ return ret;
+}
+
+#define add_literal_option(o) do { options[optnum].name = #o; \
+ options[optnum].flag = (void *)&o; options[optnum].has_arg = 1; \
+ options[optnum].val = -1; optnum++; } while (0)
+
+#define add_flag_option(n, p, v) do { options[optnum].name = n; \
+ options[optnum].flag = (void *)p; options[optnum].has_arg = 0; \
+ options[optnum].val = v; optnum++; } while (0)
+
+static struct option options[100];
+int optnum;
+static int verbose;
+
+int options_init()
+{
+ optnum = 0;
+ /* on gcc 64, pointer to variable can be used only on run-time
+ */
+ memset(options, 0, sizeof(options));
+ add_literal_option(io_type);
+ add_literal_option(buf_size);
+ add_literal_option(ioctl_num);
+ add_literal_option(ioctl_type);
+ add_literal_option(loops);
+ add_literal_option(delay);
+ add_literal_option(offset);
+ add_flag_option("ioctl", &io_type, ioctl_io);
+ add_flag_option("mmap", &io_type, mmap_io);
+ add_flag_option("file", &io_type, file_io);
+ add_flag_option("ignore_eof", &ignore_eof, 1);
+ add_flag_option("verbose", &verbose, 1);
+ add_flag_option("ro", &ro, 1);
+ add_flag_option("wo", &wo, 1);
+ options[optnum].name = strdup("help");
+ options[optnum].has_arg = 0;
+ options[optnum].val = 'h';
+ optnum++;
+ return optnum;
+}
+
+/*
+ * expand_arg, return_if_arg_is_equal - utility functions
+ * to translate command line parameters
+ * from string to numeric values using predefined preprocessor defines
+ */
+
+#define return_if_arg_is_equal(entry) do { if (0 == strcmp(arg, #entry)) return entry; } while (0)
+
+int expand_arg(char *arg)
+{
+ if (!arg)
+ return 0;
+/*
+ return_if_arg_is_equal(SOCK_STREAM);
+*/
+ return strtol(arg, NULL, 0);
+}
+
+char *usage = "dio - Device Input/Output utility\n\
+Usage:\n\
+ dio <options> <device file>\n\
+\n\
+options:\n\
+\n\
+default values are marked with '*'\n\
+\n\
+ -h | --help\n\
+ show this help\n\
+\n\
+ --buf_size <n> \n\
+ I/O buffer size\n\
+\n\
+Samples:\n\
+\n\
+TBD\n\
+\n\
+";
+
+int init(int argc, char *argv[])
+{
+ int opt = 0;
+ int longindex = 0;
+ options_init();
+ opterr = 0;
+ while ((opt = getopt_long(argc, argv, "h", options, &longindex)) != -1) {
+ switch (opt) {
+ case 0:
+ if (options[longindex].val == -1)
+ *options[longindex].flag = expand_arg(optarg);
+ break;
+ case 'h':
+ printf("%s", usage);
+ exit(0);
+ break;
+ default: /* '?' */
+ printf("Error in arguments\n");
+ exit(EXIT_FAILURE);
+ }
+ }
+ if (optind < argc)
+ dev_name = argv[optind];
+ if (io_type == ioctl_io && buf_size >= 1 << _IOC_SIZEBITS)
+ fprintf(stderr, "WARNING: size of ioctl data it too big\n");
+ return 0;
+}
+
+int main(int argc, char *argv[])
+{
+ int dev;
+
+ buf_size = sysconf(_SC_PAGESIZE);
+ init(argc, argv);
+ verbose && fprintf(stderr, "%s compiled " __DATE__ " " __TIME__ "\n", argv[0]);
+ if (io_type == ioctl_io && buf_size >= 1 << _IOC_SIZEBITS)
+ buf_size = (1 << _IOC_SIZEBITS) - 1;
+ inbuf = malloc(buf_size);
+ outbuf = malloc(buf_size);
+ dev = open(dev_name, O_CREAT | O_RDWR, 0666);
+ if (io_type == mmap_io) {
+ mm = mmap(NULL, buf_size, PROT_READ | PROT_WRITE, MAP_SHARED, dev,
+ offset & ~(sysconf(_SC_PAGESIZE)-1));
+ if (mm == MAP_FAILED) {
+ warn("mmap() failed");
+ goto exit;
+ }
+ mem = mm + (offset & (sysconf(_SC_PAGESIZE)-1));
+ }
+ if (verbose) {
+ trace_dec(io_type);
+ trace_dec(buf_size);
+ trace_dec(ignore_eof);
+ trace_dec(verbose);
+ }
+ switch (io_type) {
+ case mmap_io:
+ case ioctl_io:
+ if (!ro) {
+ ret = read(fileno(stdin), inbuf, buf_size);
+ if (ret < 0)
+ goto exit;
+ ret = output(dev, inbuf, ret);
+ }
+ if (!wo) {
+ ret = input(dev, outbuf, buf_size);
+ if (ret < 0)
+ goto exit;
+ write(fileno(stdout), outbuf, ret);
+ }
+ break;
+ case file_io:
+ default:
+ io_start(dev);
+ }
+exit:
+ if (mm && mm != MAP_FAILED)
+ munmap(mm, buf_size);
+ free(outbuf);
+ free(inbuf);
+ close(dev);
+ exit(EXIT_SUCCESS);
+}
diff --git a/tools/testing/ldt/ldt-test b/tools/testing/ldt/ldt-test
new file mode 100755
index 0000000..421b1e8
--- /dev/null
+++ b/tools/testing/ldt/ldt-test
@@ -0,0 +1,142 @@
+#!/bin/bash
+
+#
+# LDT - Linux Driver Template
+#
+# Test script for driver samples/ldt/
+#
+# Copyright (C) 2012 Constantine Shulyupin http://www.makelinux.net/
+#
+# Dual BSD/GPL License
+#
+
+RED="\\033[0;31m"
+NOCOLOR="\\033[0;39m"
+GREEN="\\033[0;32m"
+GRAY="\\033[0;37m"
+
+set -o errtrace
+debugfs=`grep debugfs /proc/mounts | awk '{ print $2; }'`
+tracing=$debugfs/tracing
+
+tracing()
+{
+ sudo sh -c "cd $tracing; $1" || true
+}
+
+tracing_start()
+{
+ tracing "echo :mod:ldt > set_ftrace_filter"
+ tracing "echo function > current_tracer" # need for draw_functrace.py
+ #tracing "echo function_graph > current_tracer"
+ tracing "echo 1 > function_profile_enabled"
+ # useful optional command:
+ #tracing "echo XXX > set_ftrace_notrace"
+ #sudo cat $tracing/current_tracer
+ #sudo cat $tracing/set_ftrace_filter
+ #sudo cat $tracing/function_profile_enabled
+ # available_filter_functions
+ # echo $$ > set_ftrace_pid
+}
+
+tracing_stop()
+{
+ ( echo Profiling data per CPU
+ tracing "cat trace_stat/function*" )> trace_stat.log && echo trace_stat.log saved
+ tracing "echo 0 > function_profile_enabled"
+ sudo cp $tracing/trace ftrace.log && echo ftrace.log saved
+ sudo dd iflag=nonblock if=$tracing/trace_pipe 2> /dev/null > trace_pipe.log || true && echo trace_pipe.log saved
+ tracing "echo nop > current_tracer"
+ #export PYTHONPATH=/usr/src/linux-headers-$(uname -r)/scripts/tracing/
+ # draw_functrace.py needs function tracer
+ python /usr/src/linux-headers-$(uname -r)/scripts/tracing/draw_functrace.py \
+ < trace_pipe.log > functrace.log && echo functrace.log saved || true
+}
+
+# sudo rmmod parport_pc parport ppdev lp
+sudo dmesg -n 7
+sudo rmmod ldt ldt_plat_dev 2> /dev/null
+sudo dmesg -c > /dev/null
+stty -F /dev/ttyS0 115200
+make -s
+set -o errexit
+
+#
+# Check for presence looback on /dev/ttyS0.
+# If loopback is not present, switch loopback on in the driver
+#
+
+data='loopback?'
+received=`echo $data | ./dio --ignore_eof --loops 2 --delay 10 /dev/ttyS0 2> /dev/null`
+if [ "$data" == "$received" ]; then
+echo -e "Loopback on /dev/ttyS0 detected"
+loopback=0
+else
+echo -e "No loopback behind /dev/ttyS0 detected, running ldt driver with UART in loopback mode"
+loopback=1
+fi
+
+# clean data
+echo | ./dio --ignore_eof --loops 10 --delay 10 /dev/ttyS0 2> /dev/null > /dev/null
+
+sudo modprobe ldt loopback=$loopback
+sudo modprobe ldt_plat_dev
+
+tracing_start || true
+sudo sh -c "chmod go+rw /dev/ldt*"
+data=123rw
+echo $data > /dev/ldt
+sleep 0.5
+received=`dd iflag=nonblock if=/dev/ldt 2> /dev/null || true`
+if [ "$data" == "$received" ]; then
+echo -e "${GREEN}LDT nonblocking read/write test passed$NOCOLOR"
+else
+echo -e "${RED}LDT nonblock read/write test failed$NOCOLOR"
+echo expected $data
+echo received $received
+fi
+
+data=123bl
+cat /dev/ldt > R.tmp &
+sleep 0.5; echo $data > /dev/ldt;
+sleep 0.5
+kill %1; wait %1 2> /dev/null || true
+received=`cat R.tmp`
+rm -f R.tmp
+
+if [ "$data" == "$received" ]; then
+echo -e "${GREEN}LDT blocking read/write test passed$NOCOLOR"
+else
+echo -e "${RED}LDT blocking read/write test failed$NOCOLOR"
+echo expected $data
+echo received $received
+fi
+
+data=123mmap
+received=`sudo echo $data | ./dio --mmap /dev/ldt`
+if [ "$data" == "$received" ]; then
+echo -e "${GREEN}LDT mmap test passed$NOCOLOR"
+else
+echo -e "${RED}LDT mmap test failed$NOCOLOR"
+echo expected $data
+echo received $received
+fi
+
+data=123ioctl
+received=`sudo echo $data | ./dio --ioctl /dev/ldt`
+if [ "$data" == "$received" ]; then
+echo -e "${GREEN}LDT ioctl test passed$NOCOLOR"
+else
+echo -e "${RED}LDT ioctl test failed$NOCOLOR"
+echo expected $data
+echo received $received
+fi
+
+sudo ls -l /sys/kernel/debug/ldt
+#grep ldt /proc/interrupts || true
+
+#sudo rmmod ldt ldt_plat_dev 2> /dev/null
+
+tracing_stop || true
+sudo dmesg --notime --show-delta --read-clear 2>/dev/null > kernel.log || \
+sudo dmesg -c > kernel.log && echo kernel.log saved
--
1.7.9.5


Greg KH <gregkh@...>
 

On Thu, Nov 15, 2012 at 09:22:17PM +0200, Constantine Shulyupin wrote:
From: Constantine Shulyupin <const@...>

LDT is useful for Linux driver development beginners,
hackers and as starting point for a new drivers.
Really? I strongly doubt it. The odds that a new driver needs to use a
misc device is quite low these days. Normally you just start with a
driver for a device like the one you need to write and modify it from
there. The days when people write char device drivers are pretty much
over now.

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, single misc device,
Device Model, configfs, UART 0x3f8,
HW loopback, SW loopback, ftracer.
That's a whole lot of different things all mushed together here. If you
_really_ want to make something useful, you would do individual drivers
for each of these different things, not something all tied together in a
way that no one is ever going to use.

--- /dev/null
+++ b/samples/ldt/ldt.c
@@ -0,0 +1,716 @@
+/*
+ * LDT - Linux Driver Template
+ *
+ * Copyright (C) 2012 Constantine Shulyupin http://www.makelinux.net/
+ *
+ * GPL License
GPLv1? Cool, I haven't seen that for years and years. Oh, and that's
not compatible with GPLv2, sorry.

In short, be explicit.

+ * The driver demonstrates usage of following Linux facilities:
+ *
+ * Linux kernel module
+ * file_operations
+ * read and write (UART)
+ * blocking read and write
+ * 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)
I thought you took this out.

+ * debugfs
+ * platform_driver and platform_device in another module
+ * simple UART driver on port 0x3f8 with IRQ 4
+ * Device Model
Really? I thought this was gone too. And it's not something that a
"normal" driver needs.

+ * Power Management (dev_pm_ops)
+ * Device Tree (of_device_id)
Again, that's a lot of non-related things all together in one piece,
making it hard to understand, and review, which does not lend itself to
being a good example for anything.

+/*
+ * ldt_received
+ * Called from tasklet, which is fired from ISR or timer,
+ * with data received from HW port
+ */
+
+static void ldt_received(char data)
+{
+ kfifo_in_spinlocked(&drvdata->in_fifo, &data,
+ sizeof(data), &drvdata->fifo_lock);
+ wake_up_interruptible(&drvdata->readable);
+}
As others pointed out, if you are going to use function block comments,
use the correct kerneldoc style, don't invent your own, as I never want
to see this in any driver submitted for inclusion.

+/*
+ * work section
+ *
+ * empty template function for deferred call in scheduler context
+ */
+
+static int ldt_work_counter;
Again, as others pointed out, you never want static data in a driver.

+static void ldt_work_func(struct work_struct *work)
+{
+ ldt_work_counter++;
+}
No locking? Not good.

+static int ldt_open(struct inode *inode, struct file *file)
+{
+ pr_debug("%s: from %s\n", __func__,current->comm);
+ pr_debug("imajor=%d iminor=%d \n", imajor(inode), iminor(inode));
+ pr_debug("f_flags & O_NONBLOCK=0x%X\n", file->f_flags & O_NONBLOCK);
+ /* client related data can be allocated here and
+ stored in file->private_data */
+ return 0;
+}
What's with all of the pr_debug() calls? Why? Why pick only those
specific things out of the file structure?

Also, you didn't run this through checkpatch.pl, like was requested.

+#define trace_ioctl(nr) pr_debug("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))
That's just horrid :(

+static DEFINE_MUTEX(ioctl_lock);
Why?

+static long ldt_ioctl(struct file *f, unsigned int cmnd, unsigned long arg)
No, no new ioctls, don't even think about it, sorry.

+static int ldt_cleanup(struct platform_device *pdev)
+{
+ struct ldt_data *drvdata = platform_get_drvdata(pdev);
+ dev_dbg(&pdev->dev, "%s\n", __func__);
That's a tracing function, I thought you got rid of these? Hint,
anything with __func__ in it should be removed.

+ if (!IS_ERR_OR_NULL(debugfs))
+ debugfs_remove(debugfs);
Why check this?

--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1779,10 +1779,10 @@ sub process {
$rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
!($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:|,|\)\s*;)\s*$/ ||
$line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
- $length > 80)
+ $length > 90)
Heh, nice try, that's not going to happen.

{
WARN("LONG_LINE",
- "line over 80 characters\n" . $herecurr);
+ "line ".$length." over 90 characters\n" . $herecurr);
What are you trying to do here? Why are you touching this script at
all?

--- /dev/null
+++ b/tools/testing/ldt/ldt-test
@@ -0,0 +1,142 @@
+#!/bin/bash
+
+#
+# LDT - Linux Driver Template
+#
+# Test script for driver samples/ldt/
+#
+# Copyright (C) 2012 Constantine Shulyupin http://www.makelinux.net/
+#
+# Dual BSD/GPL License
+#
+
+RED="\\033[0;31m"
+NOCOLOR="\\033[0;39m"
+GREEN="\\033[0;32m"
+GRAY="\\033[0;37m"
+
+set -o errtrace
+debugfs=`grep debugfs /proc/mounts | awk '{ print $2; }'`
+tracing=$debugfs/tracing
+
+tracing()
+{
+ sudo sh -c "cd $tracing; $1" || true
+}
sudo? Why?


+
+tracing_start()
+{
+ tracing "echo :mod:ldt > set_ftrace_filter"
+ tracing "echo function > current_tracer" # need for draw_functrace.py
+ #tracing "echo function_graph > current_tracer"
+ tracing "echo 1 > function_profile_enabled"
+ # useful optional command:
+ #tracing "echo XXX > set_ftrace_notrace"
+ #sudo cat $tracing/current_tracer
+ #sudo cat $tracing/set_ftrace_filter
+ #sudo cat $tracing/function_profile_enabled
+ # available_filter_functions
+ # echo $$ > set_ftrace_pid
+}
Is this really needed?

+
+tracing_stop()
+{
+ ( echo Profiling data per CPU
+ tracing "cat trace_stat/function*" )> trace_stat.log && echo trace_stat.log saved
+ tracing "echo 0 > function_profile_enabled"
+ sudo cp $tracing/trace ftrace.log && echo ftrace.log saved
+ sudo dd iflag=nonblock if=$tracing/trace_pipe 2> /dev/null > trace_pipe.log || true && echo trace_pipe.log saved
+ tracing "echo nop > current_tracer"
+ #export PYTHONPATH=/usr/src/linux-headers-$(uname -r)/scripts/tracing/
+ # draw_functrace.py needs function tracer
+ python /usr/src/linux-headers-$(uname -r)/scripts/tracing/draw_functrace.py \
+ < trace_pipe.log > functrace.log && echo functrace.log saved || true
+}
+
+# sudo rmmod parport_pc parport ppdev lp
Why commented out?

+sudo dmesg -n 7
+sudo rmmod ldt ldt_plat_dev 2> /dev/null
+sudo dmesg -c > /dev/null
+stty -F /dev/ttyS0 115200
Why did you just change my serial port line settings?

What is the goal of this script in the first place?

confused,

greg k-h


Bjørn Mork <bjorn@...>
 

Greg KH <gregkh@...> writes:

Normally you just start with a
driver for a device like the one you need to write and modify it from
there.
Yes.

Even if the template driver is fixed up to be the most beautiful driver
ever made, it will still always be made for non-existing hardware. This
causes two major problems:
- the driver will not be tested, so it will have bugs
- the driver will not be used by anyone, so it will not be maintained
(remember that it is initially perfect, so there is no reason to
change it)

May I suggest another approach? How about selecting a set of existing
drivers which are suitable as templates, and put all this effort into
making those drivers *the* perfect examples instead? Start submitting
cleanup patches for the selected drivers until everyone is satisfied and
then document them as starting points for anyone wanting to write a
similar driver.

I believe many subsystem maintainers already have such sample drivers
which they point new submitters to when asked. That does not mean that
these drivers necessarily are perfect, so there is still work to do here
for anyone interested. And collecting this information and documenting
it would be useful in itself.

It would also be nice if hardware availability was considered when
selecting the sample drivers. Buying an already supported device to
experiment with its driver can be useful even if you have another device
you want to write a driver for. Or just for the learning experience.

Just my € .02


Bjørn


Constantine Shulyupin
 

On Fri, Nov 16, 2012 at 11:46 AM, Bjørn Mork <bjorn@...> wrote:
Greg KH <gregkh@...> writes:

Normally you just start with a
driver for a device like the one you need to write and modify it from
there.
Yes.

Even if the template driver is fixed up to be the most beautiful driver
ever made, it will still always be made for non-existing hardware. This
causes two major problems:
- the driver will not be tested, so it will have bugs
- the driver will not be used by anyone, so it will not be maintained
(remember that it is initially perfect, so there is no reason to
change it)
Thanks. I seems you have missed.
The main advantage of LDT - is working driver with real HW and simple
test suite.
It implements trivial UART driver just to write to port, receive real
HW interrupt
and read data from port. (You can to suggest to use any another HW).
Without available UART, LDT emulates loopback in SW for testing.
Memory buffers are used for mmap and ioctl operations.
LDT test script ldt-test and test utility dio.c configures the driver
for loopback mode, passes data to the driver, receives back and
compares with input and gives result of comparison.
To perform validation tests, regression tests need simply to run test script.
Detailed kernel log and ftrace log during the test are saved for analysts.

May I suggest another approach? How about selecting a set of existing
drivers which are suitable as templates, and put all this effort into
making those drivers *the* perfect examples instead? Start submitting
cleanup patches for the selected drivers until everyone is satisfied and
then document them as starting points for anyone wanting to write a
similar driver.
Thank you. Possible too. Can you or somebody else recommend such drivers?


I believe many subsystem maintainers already have such sample drivers
which they point new submitters to when asked. That does not mean that
these drivers necessarily are perfect, so there is still work to do here
for anyone interested. And collecting this information and documenting
it would be useful in itself.
Thanks. I already research omap panda platform for improvement opportunities.

Thank you.


Greg KH <gregkh@...>
 

On Fri, Nov 16, 2012 at 10:46:18AM +0100, Bjørn Mork wrote:
Greg KH <gregkh@...> writes:

Normally you just start with a
driver for a device like the one you need to write and modify it from
there.
Yes.

Even if the template driver is fixed up to be the most beautiful driver
ever made, it will still always be made for non-existing hardware. This
causes two major problems:
- the driver will not be tested, so it will have bugs
- the driver will not be used by anyone, so it will not be maintained
(remember that it is initially perfect, so there is no reason to
change it)

May I suggest another approach? How about selecting a set of existing
drivers which are suitable as templates, and put all this effort into
making those drivers *the* perfect examples instead? Start submitting
cleanup patches for the selected drivers until everyone is satisfied and
then document them as starting points for anyone wanting to write a
similar driver.
I agree, this is a much better idea. Basing any new driver on a
known-working driver is highly preferable.

thanks,

greg k-h