On Wed, May 19, 2010 at 10:16:24AM -0700, Joe Perches wrote:
> On Wed, 2010-05-19 at 10:02 -0700, Greg KH wrote:
> > On Wed, May 19, 2010 at 09:50:54AM -0700, Joe Perches wrote:
> > > You tend to get many tab indentations.
> > In this case, it doesn't really matter. The comedi layer has much
> > bigger issues, please focus on the stuff that is important.
>
> Like long line lengths, braces placements and
> checkpatch errors?
No, like wierd callbacks, non-standard module programming macros,
difficult driver model interaction, userspace api that is questionable,
improper internal kernel api calls, and the like.
> Perhaps this might be interesting:
> $ git log --pretty=oneline drivers/staging/comedi/
>
> It's all a piece. Fixes are fixes.
> Unnecessary code is unnecessary.
I agree, but this addition, to fix a bug, is not a big deal.
> > > As this is in a while (.. && !retval) the retval = 0 is unnecessary,
> > > so maybe something like this is better?
> > >
> > > if (!(comedi_get_subdevice_runflags(s) & SRF_RUNNING)) {
> > > if (count)
> > > break;
> > > if (comedi_get_subdevice_runflags(s) & SRF_ERROR)
> > > retval = -EPIPE;
> > > do_become_nonbusy(dev, s);
> > > break;
> > > }
> > >
> > > Maybe a macro/function like test_runflags might be useful
> > > to reduce the name/line length.
> > >
> > > #define test_runflags(s, flags) \
> > > return !!(comedi_get_subdevice_runflags(s) & flags);
> >
> > _NEVER_ put a code path change in a #define. That makes it hell to
> > debug.
>
> Of course a macro shouldn't have a return,
> though a function would.
Then why did you just suggest writing a macro with a return statement?
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
