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
