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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux