On Thu, 2017-01-19 at 11:00 +0300, Dan Carpenter wrote: > On Wed, Jan 18, 2017 at 11:37:55PM -0800, Michael Zoran wrote: > > This whole driver is a chicken and egg problem. The existing code > > is > > so hard to read and maintain, that it's hard to improve it in a > > incremental way. Yet, trowing large sections out the door is too > > hard > > to get seriously reviewed as well... > > > > I would like to think that what I've submitted is an improvement on > > the > > existing stuff. I'm sorry you feel that no changes are possible > > unless > > all the issues are fixed at once. > > > > This isn't fixing existing code, it's adding new code. > > So this morning, because you're asking me to reconsider this code, I > looked at it again and the function that I'm complaining about seems > totally useless. It doesn't do *anything*! I can't figure out what > it's for. It also seems totally unused. I told myself, that I must > be > missing something critical hidden in macro magic, so I applied it and > compiled it and it gives me these warnings: > > In file included from > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:52:0: > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: In > function ‘vchiq_ioctl_compat_internal’: > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h:185:38 > : warning: unused variable ‘debug_ptr’ [-Wunused-variable] > #define DEBUG_INITIALISE(local) int *debug_ptr = (local)->debug; > ^ > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:1222:2: > note: in expansion of macro ‘DEBUG_INITIALISE’ > DEBUG_INITIALISE(g_state.local) > ^ > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: At top > level: > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:1212:1: > warning: ‘vchiq_ioctl_compat_internal’ defined but not used [- > Wunused-function] > vchiq_ioctl_compat_internal( > ^ > LD [M] drivers/staging/vc04_services/vchiq.o > > I shouldn't have to review code like this when the compiler already > tells you it's unacceptable. > > regards, > dan carpenter > Thanks for taking a second look. Yes, it doesn't do anything. It's meant to be a placeholder for adding other ioctls. Patch #3 begins adding calls to this function and after adding #3 it should make more sense. Perhaps I should have combined #2 and #3 into one patch. _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
