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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
