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.
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,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
