----- Forwarded message from Dan Carpenter <dan.carpenter at oracle.com> -----
> Date: Fri, 10 May 2013 01:49:12 +0300
> From: Dan Carpenter <dan.carpenter at oracle.com>
> To: William Hubbs <w.d.hubbs at gmail.com>
> Cc: devel at driverdev.osuosl.org
> Subject: Re: speakup issues
>
> Btw, this code isn't horrible. It uses small functions and it's
> readable. It's just needs small cleanups throughout...
>
> Get rid of the global variable called "synth". That's a bad name
> for a global and it's shadowed by local variables named "synth" so
> it creates confusion.
>
> Don't do "pr_warn("synth probe\n");" on the success path.
>
> As much as possible get rid of forward declarations.
>
> Some of the 80 character line breaks were done in a funny way.
>
> What is this code?
> #if defined(__powerpc__) || defined(__alpha__)
> cval >>= 8;
> #else /* !__powerpc__ && !__alpha__ */
> cval >>= 4;
> #endif /* !__powerpc__ && !__alpha__ */
>
> Delete commented out code.
>
> The file scoped variable "timeouts" is only used in one function.
> It could be declared as a static variable locally in that function.
>
> /*
> * this is cut&paste from 8250.h. Get rid of the structure, the definitions
> * and this whole broken driver.
> */
>
> We go to a lot of effort to print out this message which just tells
> you that adds no information:
>
> if (failed) {
> pr_info("%s: not found\n", synth->long_name);
> return -ENODEV;
> }
>
> synth_add() has a memory corrupting off-by-one bug.
>
> The code returns -1 (which means "permission denied") instead of
> returning standard error codes.
>
> The author of this code doesn't like break statements and uses
> compound conditions to avoid them.
>
> 426 for (i = 0; i < MAXSYNTHS && synths[i] != NULL; i++)
> 427 /* synth_remove() is responsible for rotating the array down */
> 428 if (in_synth == synths[i]) {
> 429 mutex_unlock(&spk_mutex);
> 430 return 0;
> 431 }
> 432 if (i == MAXSYNTHS) {
> 433 pr_warn("Error: attempting to add a synth past end of array\n");
> 434 mutex_unlock(&spk_mutex);
> 435 return -1;
> 436 }
>
> Unless there is a special reason then for loops should be written
> in the canonical format. Use curly braces for readability when
> there is a multi-line loop even if they are not needed
> syntactically.
>
> for (i = 0; i < ARRAY_SIZE(synths); i++) {
> /* synth_remove() is responsible for rotating the array down */
> if (synths[i] == in_synth) {
> mutex_unlock(&spk_mutex);
> return 0;
> }
> if (synths[i] == NULL)
> break;
> }
> if (i == ARRAY_SIZE(synths)) {
> pr_warn("Error: attempting to add a synth past end of array\n");
> mutex_unlock(&spk_mutex);
> return -ENOMEM;
> }
>
> Pretty much where ever you look there is something to clean up.
> Fortunately, it's mostly small things.
>
> Sparse has a few things it complains about.
>
> regards,
> dan carpenter
----- End forwarded message -----
