Re: [PATCH RFC] staging: comedi: dt282x: condition with no effect - if identical to else

[Ian Abbott <abbotti@mev.co.uk>]

On 03/02/15 12:38, Nicholas Mc Guire wrote:
> The if and the else branch code are identical - so the condition has no
> effect on the effective code - this patch removes the condition and the
> duplicated code.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> ---
>
> The if and else branch are identical code thus the condition has no effect
>
>      if (cmd->scan_begin_src == TRIG_FOLLOW) {
>              /* internal trigger */
>              err |= cfc_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
>      } else {
>              /* external trigger */
>              /* should be level/edge, hi/lo specification here */
>              err |= cfc_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
>      }
>
> As the comments indicate that they are serving different purposes this
> looks like a bug. In any case - if intentional - it would need some
> comments on why.
>
> This needs a review by someone that knows the details of this driver.
> Also not sure about the retained comment string if that is still valid now.
>
> Patch was only compile tested for x86_64_defconfig + CONFIG_STAGING=y
> CONFIG_COMEDI=m, COMEDI_ISA_DRIVERS=y, CONFIG_COMEDI_DT282X=m
>
> Patch is against 3.0.19-rc7 (localversion = -next-20150203)
>
>   drivers/staging/comedi/drivers/dt282x.c |   10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/dt282x.c b/drivers/staging/comedi/drivers/dt282x.c
> index 051dfb2..22c59e5 100644
> --- a/drivers/staging/comedi/drivers/dt282x.c
> +++ b/drivers/staging/comedi/drivers/dt282x.c
> @@ -685,14 +685,8 @@ static int dt282x_ai_cmdtest(struct comedi_device *dev,
>
>   	err |= cfc_check_trigger_arg_is(&cmd->start_arg, 0);
>
> -	if (cmd->scan_begin_src == TRIG_FOLLOW) {
> -		/* internal trigger */
> -		err |= cfc_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
> -	} else {
> -		/* external trigger */
> -		/* should be level/edge, hi/lo specification here */

I think what that comment means is that it should allow scan_begin_arg 
to have various combinations of the CR_EDGE and CR_INVERT bits set. 
I.e. it ought to allow whatever combination of CR_EDGE and CR_INVERT 
better describes the nature of the external trigger signal, in addition 
to allowing the lazy default value 0.

I don't know what the nature of the external trigger signal is, as I 
haven't seen the manual.  I think Hartley might have seen one.

> -		err |= cfc_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
> -	}
> +	/* internal trigger */

That comment would be misleading as it could be an internal or external 
trigger.

If you want to apply this patch, remove that comment first.  But I'd 
rather leave the existing code there as a reminder.

> +	err |= cfc_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
>
>   	err |= cfc_check_trigger_arg_min(&cmd->convert_arg, 4000);
>
>


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