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

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

[Nicholas Mc Guire]

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 */
-		err |= cfc_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
-	}
+	/* internal trigger */
+	err |= cfc_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
 
 	err |= cfc_check_trigger_arg_min(&cmd->convert_arg, 4000);
 
-- 
1.7.10.4

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

[Ian Abbott]

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/  )=-

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

[Nicholas Mc Guire]

On Tue, 03 Feb 2015, Ian Abbott wrote:

> 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.
>

yup - was unsure on what to put there (if any).

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

if that is the preferred solution maybe someone with suitable
knowledge could add a comment in the else case explaining why
it is the same or at least making it clear that it is an
intentional placeholder.

>> +    err |= cfc_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
>>
>>      err |= cfc_check_trigger_arg_min(&cmd->convert_arg, 4000);
>>
>>
>
thanks for your review comments!

thx!
hofrat

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

[Hartley Sweeten]

On Tuesday, February 03, 2015 8:13 AM, Ian Abbott wrote:
> 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);
>>      }
>>
> 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.

According to the manual, the external trigger is not "programmable". It's
a Schmitt trigger input, enables on TTL logic low, with a 22K pullup.

Since the 'scan_begin_arg' is not actually used for the analog input async
command, I think removing the comments completely is fine. Just change
the check to:

	err |= cfc_check_trigger_arg_is(&cmd->scan_begin_arg, 0);

Regards,
Hartley

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

[Nicholas Mc Guire]

On Wed, 04 Feb 2015, Hartley Sweeten wrote:

> On Tuesday, February 03, 2015 8:13 AM, Ian Abbott wrote:
> > 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);
> >>      }
> >>
> > 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.
> 
> According to the manual, the external trigger is not "programmable". It's
> a Schmitt trigger input, enables on TTL logic low, with a 22K pullup.
> 
> Since the 'scan_begin_arg' is not actually used for the analog input async
> command, I think removing the comments completely is fine. Just change
> the check to:
> 
> 	err |= cfc_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
>

thanks for that clarification - will fix it up and resend.

thx!
hofrat