Re: [PATCH 2/9] VC04_SERVICES: Add top level compat ioctl handler

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux