On Wed, Apr 23, 2014 at 08:39:06AM +0800, Michalis Pappas wrote: > After all patches have been applied, the only remaining issue on the > TODO list is to conform to the coding standards. The remaining issues > reported by checkpatch.pl are probably pedantic, so if agreed, that > task can be removed from the list too. So I did a: for i in $(find drivers/staging/gdm72xx/ -name \*.c) ; do ./scripts/checkpatch.pl --strict -f $i 2>&1 ; done | tee err-list I didn't look through all the issues but the ones I looked at seemed valid. The "no space after a cast" rule is good because cast precedence used to be a common source of bugs. The code in extract_qos_list() was badly mangled to get around the 80 character limit and checkpatch.pl finds that. That function is very ugly. Just reverse the if statements instead of jamming the code up against the far right side of the screen. if (!qcb->csr[i].enabled) continue; if (qcb->csr[i].qos_buf_count >= qcb->qos_limit_size) continue; if (list_empty(&qcb->qos_list[i])) continue; Also why is returning a u32? Kernel style is int. But this doesn't return errors and the callers don't check so it should be void. This driver suffers from prefering u32 over int in many places. This was my only static checker warning (harmless): drivers/staging/gdm72xx/gdm_wimax.c:780 gdm_wimax_transmit_pkt() warn: we tested 'len' before and it was 'true' In gdm_usb_send(), the "BUG_ON(len > TX_BUF_SIZE - padding - 1);" should be proper error handling. I didn't really do a proper review of this code when I saw all the checkpatch.pl complaints still. regards, dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
