[PATCH 1/3] staging: comedi: dt282x: remove unnecessary comedi_check_trigger_arg_max()

[Ian Abbott]

On 23/03/16 16:50, Hartley Sweeten wrote:
> On Wednesday, March 23, 2016 3:47 AM, Ian Abbott wrote:
>> On 22/03/16 17:01, H Hartley Sweeten wrote:
>>> Step 4 of the (*do_cmdtest) calls dt282x_ns_to_timer() to work out the
>>> divisor needed to generate the 'convert_arg' timing. If the 'convert_arg'
>>> timing can not be generated, the function returns the max timing that can
>>> be generated.
>>>
>>> The Step 3 'max' check of the 'convert_arg' isn't necessary.
>>>
>>> Signed-off-by: H Hartley Sweeten <hsweeten at visionengravers.com>
>>> Cc: Ian Abbott <abbotti at mev.co.uk>
>>> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>>> ---
>>>    drivers/staging/comedi/drivers/dt282x.c | 2 --
>>>    1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/comedi/drivers/dt282x.c b/drivers/staging/comedi/drivers/dt282x.c
>>> index 06a87a1..be6a0bc 100644
>>> --- a/drivers/staging/comedi/drivers/dt282x.c
>>> +++ b/drivers/staging/comedi/drivers/dt282x.c
>>> @@ -689,8 +689,6 @@ static int dt282x_ai_cmdtest(struct comedi_device *dev,
>>>
>>>    	err |= comedi_check_trigger_arg_min(&cmd->convert_arg, 4000);
>>>
>>> -#define SLOWEST_TIMER	(250*(1<<15)*255)
>>> -	err |= comedi_check_trigger_arg_max(&cmd->convert_arg, SLOWEST_TIMER);
>>>    	err |= comedi_check_trigger_arg_min(&cmd->convert_arg, board->ai_speed);
>>>    	err |= comedi_check_trigger_arg_is(&cmd->scan_end_arg,
>>>    					   cmd->chanlist_len);
>>>
>>
>> I'd be inclined to keep the existing check in place, as it goes with the
>> arg_min test (although one of the arg_min tests on convert_arg is
>> redundant, as you point out in patch 2).
>
> It's just unnecessary. And the #define in the function is a bit ugly in my opinion.

The upper and lower bounds on the timings are usually checked at step 3. 
  That's the case in amplc_pci230, cb_das16_cs, comedi_rt_timer, 
dmm32at, dt2814, ni_pcidio, etc.

> If it's removed the net effect is still the same. Only instead of the arg_max test
> happening in the trivial validation (step 3) it will happen as a side effect of the
> divisor calculation (step 4).  If the user passes a convert_arg larger than
> 250*(1<<15)*255 (2,088,960,000 ns) it will still get changed to the acceptable
> max value.

It could affect the number of test operations that need to be performed 
on a particular command to get it to pass.  For example, if convert_arg 
is out of bounds and there is some other problem at step 3, but no other 
problems at step 4, the old code would fail at step 3 and a subsequent 
call would pass step 4, but the new code would fail at step 3 and a 
subsequent call would also fail at step 4 (because convert_arg wasn't 
adjusted by step 3).

> But, if you prefer to keep it in I will modify this patch to just fix the
> checkpatch.pl issue.

I prefer to keep the test for consistency, but you can make it less ugly 
if you want.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti at mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-