[PATCH 00/22] staging: comedi: das800: cleanup driver

Ian Abbott abbotti at mev.co.uk
Tue Apr 23 13:12:38 UTC 2013


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 at mev.co.uk>

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti at mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-



More information about the devel mailing list