On 2013-04-23 17:21, H Hartley Sweeten wrote:
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.
Yes, you're correct.
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.
Alternatively, the attach could be made to fail if the probed board
differs from the supplied board (as far as can be detected). Something
like:
board = das800_probe(dev);
if (board < 0 || thisboard != &das800_board[board]) {
dev_err(dev->dev->class, "mismatched probed board type\n");
return -ENODEV;
}
But it's probably better to stick with the current behaviour for
backwards compatibility.
Also, das800_probe() could return an actual pointer to a board, or NULL
(or an ERR_PTR(-errno) value) on error.
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.
A mutex can't be used because a mutex can sleep, which isn't allowed
when called from an interrupt routine.
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
--
-=( 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
