On Tuesday, April 23, 2013 6:13 AM, Ian Abbott wrote:
> On 2013-04-23 02:31, H Hartley Sweeten wrote:
>> Tidy up this comedi legacy driver and make it checkpatch.pl clean.
>>
>> H Hartley Sweeten (22):
>> staging: comedi: das800: move module_{init,exit} to end of file
>> staging: comedi: das800: move das800_attach()
>> staging: comedi: das800: move das800_probe()
>> staging: comedi: das800: move das800_set_frequency()
>> staging: comedi: das800: remove forward declarations
>> staging: comedi: das800: introduce das800_ind_{write,read}()
>> staging: comedi: das800: cleanup range table declarations
>> staging: comedi: das800: cleanup the boardinfo
>> staging: comedi: das800: remove 'volatile' on private data variables
>> staging: comedi: das800: tidy up das800_ai_do_cmdtest()
>> staging: comedi: das800: interrupts are required for async command support
>> staging: comedi: das800: allow attaching without interrupt support
>> staging: comedi: das800: tidy up subdevice init
>> staging: comedi: das800: rename {enable,disable}_das800
>> staging: comedi: das800: remove extra divisor calculation call
>> staging: comedi: das800: tidy up das800_do_insn_bits()
>> staging: comedi: das800: tidy up das800_di_insn_bits()
>> staging: comedi: das800: tidy up das800_ai_insn_read()
>> staging: comedi: das800: tidy up das800_interrupt()
>> staging: comedi: das800: tidy up the private data
>> staging: comedi: das800: rename CamelCase vars in das800_ai_do_cmd()
>> staging: comedi: das800: cleanup the cio-das802/16 fifo comments
>>
>> drivers/staging/comedi/drivers/das800.c | 965 +++++++++++++++-----------------
>> 1 file changed, 454 insertions(+), 511 deletions(-)
>>
>
> Looks okay to me. I have a few suggestions for additional changes which
> could be done later:
>
> 1. Don't bother setting dev->board_name in the attach routine as it
> should already be set.
Unfortunately, this driver does additional probing and could change the
dev->board_ptr. I feel the dev->board_name should be adjusted accordingly.
We could remove the das800_probe(). If we do I think the 'ID' register should
still be read and a message displayed if the id_bits do not match the boardinfo
that the user requested to attach to. But, still attach to the board using the
boardinfo that the user requested. This way the dev->board_ptr does not get
changed and dev->board_name stays the same.
> 2. Define an additional spinlock in the private structure for use by
> das800_ind_read() and das800_ind_write(). (Maybe call it ind_spinlock.)
> Then most of the uses of dev->spinlock can be removed, apart from the
> ones to do with interrupt control, e.g. the CONTROL1 register is
> involved with interrupt control so should still use dev->spinlock.
> Alternatively, da800_ind_read() and das800_ind_write() could use
> dev->spinlock, but then you'd need variants of the functions for use
> when dev->spinlock is already held.
I started doing that but felt it could be done later as a separate patch.
The interrupt function is the sticky part. That function holds the spinlock
thru the entire function. The rest of the users of the indirect read/write
only hold it for the read/write operation.
I'll look at adding a private spinlock (would a mutex be better?) after
this series is in.
> 3. Fix the bug in das800_do_wbits() that enables interrupts in the
> CONTROL1 register regardless of whether interrupts are supposed to be
> enabled. Perhaps we need to keep a shadow of the desired CONTROL1
> register value in the private structure and lock dev->spinlock before
> modifying the CONTROL1 register and its shadow.
I spotted that also. The attach function also enables interrupts early when
it initializes the digital outputs. That enables should also be removed.
> Since those can be done later...
>
> Reviewed-by: Ian Abbott <abbotti@xxxxxxxxx>
Thanks!
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
