On Thu, Nov 15, 2012 at 09:22:17PM +0200, Constantine Shulyupin wrote: > From: Constantine Shulyupin <co...@makelinux.com> > > 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 _______________________________________________ Celinux-dev mailing list Celinux-dev@lists.celinuxforum.org https://lists.celinuxforum.org/mailman/listinfo/celinux-dev
