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.
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.
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.
Since those can be done later...
Reviewed-by: Ian Abbott <abbotti@xxxxxxxxx>
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
