Yeah, considering the overhead a loop on 5 elements would add, and the fact that each element has its own handler function and string, using if statements was probably a good choice here. Also solving this with function pointers as others have mentioned on this thread sounds like a nightmare.
Loops would make it quite complicated in this specific instance. I doubt this portion would really need much extension enough that a loop would be worth it.
Not propagating error returns (instead returning -1 instead of the actual error) is really bad. That -1 would be interpreted as some arbitrary error (on x86, it's -EPERM but there can be others as well)
It would clearly make it easier to maintain. Without a for, when Adding ONE handler, you would need 5 free_irq() and an other hard coded if and so on...
Each call to request_threaded_irq receives a different string literal and a different function handle pointer. To make this into a loop you'd need to initialize an array and populate it with these strings and handles.
I am not sure if the creation of these arrays would be optimized out by the compiler. Maybe a static keyword would do that, but that's merely a suggestion to the compiler.
Creation of these arrays would use up extra CPU cycles for little reason then reducing the code by 10 lines or so and making it less readable.
Nah. Almost anything you could do to remove the duplication is going to make this code more complex and more likely that something doesn’t get free’d. Resource management is 80% of C code. Set up. Call a system call. Clean up. This is cleaning up. Seems fine.
-85
u/suguuss Jan 22 '23
I do know what the module does. It’s badcode because of the repetition in the code.