On Tuesday 30 August 2005 14:19, Jesper Juhl wrote:
> On 8/30/05, Mark Gross <mgross@linux.intel.com> wrote:
> > On Tuesday 30 August 2005 13:31, Mark Gross wrote:
> > > On Tuesday 30 August 2005 12:16, Marcelo Tosatti wrote:
> > > >
> > > > Mark,
> > > >
> > > > Please fix identation accordingly to CodingStyle and repost, it
> > > > looks quite ugly at the moment.
> > > >
> > > Sorry about that.
> > >
> > 
> > My email client is f-ing with me.  See attached.
> > 
> 
> ok, a few small comments  : 
> 
> 
> +/* 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 is longer than 80 chars - there are a few more such long lines,
> not going to point them all out, just one example. Ohh, and
> "opperation" should be "operation".
> 
> +All sysfs interaces are integers in hex format, i.e echo 99 > refalign
> 
> s/interaces/interfaces/
> 
> +#if CONFIG_DEBUG_KERNEL
> 
> I believe this should be "#ifdef CONFIG_DEBUG_KERNEL" or "#if
> defined(CONFIG_DEBUG_KERNEL)" or you'll run afoul of the -Wundef
> crowd.
> 
> + int ret_val = 0;
> 
> There seems to be a preference for the name "retval" in the kernel source.
> 
> + val = (unsigned char)arg;
> 
> 
> That cast looks pointless.
> 
> +tlclk_read(struct file * filp, char __user * buf, size_t count, loff_t * f_pos)
> 
> 
> tlclk_read(struct file *filp, char __user *buf, size_t count, loff_t *f_pos)
> "*" next to the variable name is generally the preffered coding style
> (several cases of this, only pointing out one).
> 
> + val = (unsigned char)tmp;
> 
> pointless cast. Do take a look at all your casts and check if they are
> really needed and remove them if not.
> 
> + * This is also the probe opperation to avoid driver use on 
> 
> s/opperation/operation/
> 
> +/* switchover_timer.function = switchover_timeout; */
> 
> +/* switchover_timer.data = 0; */
> 
> Why submit a driver with commented out code? If this is not supposed
> to be there, then just remove it. If it needs to be added later, then
> submit a patch later to add it.
> Some people may disagree with me here, but that's my oppinion.
> 
> +      out3:
> 
> 
> labels belong at column 0 (zero).
> 
> 
Thank you for your input.  See attached.


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