Randy Dunlap <rdunlap@xenotime.net> wrote:
quoted text
> This mailing list isn't really the correct one for driver reviews for
> getting into staging/ or anywhere else in the kernel.  It's just for advice.  :)

I don't know if this is the proper place to follow up on your review,
then, but I did appreciate your advice, and would like to comment on a
few points:

quoted text
> > +# ifdef MODULE_LICENSE
> > +MODULE_LICENSE("GPL");
> > +# endif  /* MODULE_LICENSE */
>
> The module always needs a MODULE_LICENSE().  It should not be conditional.

It's conditional on MODULE_LICENSE itself; that's merely compatibility
cruft.  (I know this will have to go eventually, but I'm in no hurry.)

quoted text
>         for (i = 0; i < 8; i++)
>
> but better to have a #define for 8.

For 8-bits-in-a-byte?  Does anyone do that?  (I count nearly 500
instances of this exact loop in the current kernel.)

quoted text
> > +        for( i = 0; (i < 20) && !(ack=GET(DATA_IN)); i++ ) {
>
> What magic number is 20?  #define for it?

There are many timing values that occur only once throughout the code;
I'm not sure that replacing them all with #defines would be more
readable than a good set of comments.

Even worse, in cases such as above, the 20 is purely arbitrary.  So is
the udelay(100) in the loop.  What matters is the product, ie. we will
give up after 2000us, while polling a few times in the interval.

I'll try to look around for inspiration and see if there are other
drivers doing similar things.

quoted text
> size_t cnt should be printed with %zu format.
> Check other similar warnings. (yes, gcc warns about that)

On x86-64, I assume?  It's dead silent on i386, unfortunately.  Thanks
for the heads up!

quoted text
> Can it be GPL v2 only or is it too late to change that?

Does it matter, as long as it's not any less permissive?  I mean:

  $ git grep -l '(at your option) any later version' | wc -l
  5854

Contacting all copyright holders would be a hassle, so I just want to
make sure there's a good reason for it.


Again, thanks for your generous help!


-- 
Staff meeting in the conference room in 3 minutes.

_______________________________________________
Kernel-mentors mailing list
Kernel-mentors@selenic.com
http://selenic.com/mailman/listinfo/kernel-mentors