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

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

On 18/01/15 11:53, 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 <der.herr@hofr.at>
> ---
>
> Patch is against 3.19.0-rc4 -next-20150116
>
> This patch was only compile tested with x86_64_defconfig + CONFIG_STAGING=y
> CONFIG_COMEDI=m, CONFIG_COMEDI_PCI_DRIVERS=m, CONFIG_COMEDI_RTD520=m
>
> commit c5930d66 (staging: comedi: rtd520: use the comedi_device 'mmio'
> member) introduced the current code, the probably bug was though
> present before this commit:
>
>          if (devpriv->xfer_count > 0)    /* transfer every N samples */
>                  writew(IRQM_ADC_ABOUT_CNT, dev->mmio + LAS0_IT);
>          else                            /* 1/2 FIFO transfers */
>                  writew(IRQM_ADC_ABOUT_CNT, dev->mmio + LAS0_IT);
>
> as the if and else branch are identical but the comments indicate that
> they are serving different purposes this looks like a bug. The code has
> though been in use in this form for more than 2 years - so maybe the
> condition is simply unnecessary. In any case this needs a review
> by someone who knows the details of the driver - the below patch
> is only cleaning up the current situation and may thus be wrong.
>
> ...and how did this pass multiple modifications and reviews ?...

The two halves used to output different kernel debug messages which were 
removed in commit d62bc468.  Prior to that, in commit 60aeb080, a whole 
bunch of unused #ifdef code was removed, which made the 'else' branch 
the same as the 'if' branch, apart from the debug messages, as can be 
seen here around line 1319 on the left side:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/staging/comedi/drivers/?id=60aeb08024dcd8a392917ab72a107ad451a59030&context=5&ignorews=0&dt=1

>
>   drivers/staging/comedi/drivers/rtd520.c |    6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/rtd520.c b/drivers/staging/comedi/drivers/rtd520.c
> index 06ae436..3ccdade 100644
> --- a/drivers/staging/comedi/drivers/rtd520.c
> +++ b/drivers/staging/comedi/drivers/rtd520.c
> @@ -1012,10 +1012,8 @@ static int rtd_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
>   	readw(dev->mmio + LAS0_CLEAR);
>
>   	/* TODO: allow multiple interrupt sources */
> -	if (devpriv->xfer_count > 0)	/* transfer every N samples */
> -		writew(IRQM_ADC_ABOUT_CNT, dev->mmio + LAS0_IT);
> -	else				/* 1/2 FIFO transfers */
> -		writew(IRQM_ADC_ABOUT_CNT, dev->mmio + LAS0_IT);
> +	/* transfer every N samples */
> +	writew(IRQM_ADC_ABOUT_CNT, dev->mmio + LAS0_IT);
>
>   	/* BUG: start_src is ASSUMED to be TRIG_NOW */
>   	/* BUG? it seems like things are running before the "start" */
>

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

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