Re: [PATCH] staging: dgnc: Encapsulate global variables in a structure

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

 



On 08/11, Greg KH wrote:
> On Sun, Aug 10, 2014 at 09:19:15PM +0200, Konrad Zapalowicz wrote:
> > On 08/10, Greg KH wrote:
> > > On Sat, Aug 09, 2014 at 08:16:37PM +0200, Konrad Zapalowicz wrote:
> > > > This commit binds global variables of dgnc driver in a structure so
> > > > that it is logically consistent. The structure is accessed via getter
> > > > function and as a result the externing of globals is removed. The names
> > > > of the variables are also changed to be more eye friendly.
> > > > 
> > > > Signed-off-by: Konrad Zapalowicz <bergo.torino@xxxxxxxxx>
> > > > ---
> > > > 
> > > > This patch applies on top of my two previous patch series. In case it
> > > > is an issue I can resubmit the whole series for dgnc driver as one
> > > > patch set - just let me know.
> > > > 
> > > >  drivers/staging/dgnc/dgnc_driver.c | 82 +++++++++++++++++++-------------------
> > > >  drivers/staging/dgnc/dgnc_driver.h | 20 ++++++----
> > > >  drivers/staging/dgnc/dgnc_mgmt.c   | 48 ++++++++++++----------
> > > >  drivers/staging/dgnc/dgnc_sysfs.c  | 31 ++++++++++----
> > > >  4 files changed, 102 insertions(+), 79 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/dgnc/dgnc_driver.c b/drivers/staging/dgnc/dgnc_driver.c
> > > > index 724e4ab..9a23e9a 100644
> > > > --- a/drivers/staging/dgnc/dgnc_driver.c
> > > > +++ b/drivers/staging/dgnc/dgnc_driver.c
> > > > @@ -83,17 +83,11 @@ static const struct file_operations dgnc_BoardFops = {
> > > >  	.release	=	dgnc_mgmt_close
> > > >  };
> > > >  
> > > > -
> > > > -/*
> > > > - * Globals
> > > > - */
> > > > -uint			dgnc_NumBoards;
> > > > -struct dgnc_board		*dgnc_Board[MAXBOARDS];
> > > > -DEFINE_SPINLOCK(dgnc_global_lock);
> > > > -int			dgnc_driver_state = DRIVER_INITIALIZED;
> > > > -ulong			dgnc_poll_counter;
> > > > -uint			dgnc_Major;
> > > > -int			dgnc_poll_tick = 20;	/* Poll interval - 20 ms */
> > > > +static struct dgnc_driver driver = {
> > > > +	.lock = __SPIN_LOCK_UNLOCKED(driver.lock),
> > > > +	.state = DRIVER_INITIALIZED,
> > > > +	.poll_tick = 20,
> > > > +};
> > > 
> > > While it is great to get rid of a bunch of globals, you now just
> > > replaced it with a different one :(
> > > 
> > > This all should be dynamic, no need for a static array of devices at
> > > all.
> > > 
> > > There shouldn't be a need for the driver_state at all, and the poll_tick
> > > should be device specific, not driver-wide.  The major is fine, but it
> > > can just be a single static variable, right?
> > > 
> > > So, I'd like to see the array of devices go away, can you do that
> > > instead of just moving it elsewhere?
> > 
> > No doubt that there is a lot of room for improvement in this code. Now,
> > since I'm new to this driver and do not own the hardware yet I'm
> > taking the step-by-step approach patiently learning the code and
> > dependencies between different files of this driver.  
> 
> That's fine, and is a good thing to do.
> 
> > Now, do you accept this patch knowing that this is an ongoing process of
> > this driver refubrishment and there will be more patches (ie. I can base
> > the future work on this patch) or you prefer it to be done differently
> > (ie. I should revert and do it right from the beginning).
> 
> I think you should do this "correct" from the beginning, I don't want to
> take this patch.

Ok.
 
> How about doing the "easy" things first, like the driver-static
> variables you have here and try to get rid of them, except for
> dgnc_Major, that should just be a simple static variable.
> 
> Get rid of dgnc_driver_state, that shouldn't be needed, and the poll
> counter mess, that should be device specific, not a global for the
> driver.  Then move to the device-specific things, making them dynamic
> and living off of the device structure that the kernel gives you (in the
> tty device).
> 
> Does that sound reasonable?

Thanks, I appreciate the guidelines. I especially found intersting the
suggestion to put emphasis on using the tty device structure.

Cheers,
Konrad

> thanks,
> 
> greg k-h
_______________________________________________
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