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