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

Ian Abbott abbotti at mev.co.uk
Wed Apr 24 10:23:46 UTC 2013


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

-- 
-=( 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