On Thursday 06 October 2005 09:52, Jesper Juhl wrote:
> On 10/6/05, Mark Gross <mgross@linux.intel.com> wrote:
> >
> > Andrew,
> >
> > Attached is a simple charactor driver for possible inclusion in your MM tree.
> >
> > This driver is specific to the MPCBL0010 that will start shipping this fall.
> >
> > The telcom clock is a special circuit, line card PLL, that provids a mechanism
> > for synchronization of specialized hardware across the backplane of a chassis
> > of multiple computers with similar specail curcits.  In this case the
> > synchronization signals get routed to multiple places, typically to pins on
> > expansion slots for hardware that knows what to do with this signal.  (SONET,
> > G.813, stratum 3...) and similar signaling applications found in telcom sites
> > can use this type of thing.
> >
> > The actual device is hidden behind the FPGA on the motherboar, and is
> > connected to the FPGA via I2C.  This driver only talks to the FPGA registers.
> >
> 
> A few minor style and spelling comments :
> 
> [snip]
> > + *
> > + * Send feedback to <sebastien.bouchard@ca.kontron.com>
> > + *
> > + * 2.6 driver version being maintained by <mark.gross@intel.com>
> 
> shouldn't this info go into CREDITS/MAINTAINERS and the above then simply be
> 
>  * Send feedback to Sebastien Bouchard and the current maintainer.
> 
> ???
> 
> Same comment for the other files.
> 
> 
> [snip]
> > +/* sysFS interface definition:
> 
> Isn't it just called "sysfs" without the caps?
> 
> 
> > +Uppon loading the driver will create a sysfs directory under class/misc/tlclk.
> 
> s/Uppon/Upon/
> 
> 
> > +
> > +This directory exports the following interfaces.  There opperation is documented
> 
> Line exceeds 80 characters (in this case due to trailing whitespace).
> 
> Let me quote Documentation/CodingStyle Chapter 2 :
> 
> "
> The limit on the length of lines is 80 columns and this is a hard limit.
> 
> Statements longer than 80 columns will be broken into sensible chunks.
> Descendants are always substantially shorter than the parent and are placed
> substantially to the right. The same applies to function headers with a long
> argument list. Long strings are as well broken into shorter strings.
> "
> 
> You have both comment and code lines elsewhere in the file that exceed this.
> Please fix.
> 
> And while you are at it, please get rid of all the trailing whitespace. A
> simple sed script will do it like this :
> 
> sed -r s/"[ \t]+$"/""/ file_with_trailing_whitespace.c > fixed_file.c
> 
> [snip]
> > +All sysfs interaces are integers in hex format, i.e echo 99 > refalign
> 
> I trust you mean "interfaces" ...
> 
> [snip]
> > +#define debug_printk( args... ) printk( args)
> > +#else
> > +#define debug_printk( args... )
> 
> Why these spaces after start paren and before closing paren?
> 
> 
> [snip]
> > +tlclk_read(struct file * filp, char __user * buf, size_t count, loff_t * f_pos)
> 
> Most of your functions nicely place the '*' next to the variable name, but this
> one, the next one and a few other cases do not. Please be consistent.
> 
> 
> [snip]
> > +static CLASS_DEVICE_ATTR(enable_clk3b_output, S_IWUGO, NULL,
> > +  store_enable_clk3b_output);
> > +
> > +
> > +
> > +static ssize_t store_enable_clk3a_output(struct class_device *d,
> 
> Why all those blank lines? One should do just fine.
> 
> 
> [snip]
> > +static CLASS_DEVICE_ATTR(enable_clk3a_output, S_IWUGO, NULL,
> > +  store_enable_clk3a_output);
> > +
> > +
> > +
> > +static ssize_t store_enable_clkb1_output(struct class_device *d,
> 
> Again a lot of blank lines, and this is not the last case (just the last one
> I'm going to point out).
> 
> 
> [snip]
> > +#ifdef  CONFIG_SYSFS
> > + if( 0 > (ret = misc_register(&tlclk_miscdev )) ) {
> 
> First a style thing :
>     if (0 > (ret = misc_register(&tlclk_miscdev))) {
> 
> Secondly, wouldn't this look nicer as
>     if ((ret = misc_register(&tlclk_miscdev)) < 0) {
> or is that just me?
> 
> Personally I think I would prefer this :
>     ret = misc_register(&tlclk_miscdev);
>     if (ret < 0) {
> Looks more readable to me.
> 
> > +  printk(KERN_ERR" misc_register retruns %d \n", ret);
> 
> Ehh, why a space just before the newline in this printk?
> 
> 
> > +  ret =  -EBUSY;
> 
>      ret = -EBUSY;
> 
> 
> 
> 
> --
> Jesper Juhl <jesper.juhl@gmail.com>
> Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
> Plain text mails only, please      http://www.expita.com/nomime.html
> 

Thanks for the review, I thought I had the 80 line thing taken care of last month.  :(

See attached, I hope it catches all the issues you found.



-- 
--mgross
BTW: This may or may not be the opinion of my employer, more likely not.