[PATCH 28/33] staging: comedi: cb_das16_cs: cleanup das16cs_ai_rinsn()

[PATCH 28/33] staging: comedi: cb_das16_cs: cleanup das16cs_ai_rinsn()

[H Hartley Sweeten]

Cleanup to analog input read function.

1) Initialize the chan, range, and aref locale variables when
   they are declared.
2) Remove need for the static local variable.
3) Remove the unnecessary cast of inw()'s return value.

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: Frank Mori Hess <fmhess@users.sourceforge.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/staging/comedi/drivers/cb_das16_cs.c | 30 ++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/comedi/drivers/cb_das16_cs.c b/drivers/staging/comedi/drivers/cb_das16_cs.c
index 11dc9b1..80e3540 100644
--- a/drivers/staging/comedi/drivers/cb_das16_cs.c
+++ b/drivers/staging/comedi/drivers/cb_das16_cs.c
@@ -111,16 +111,11 @@ static int das16cs_ai_rinsn(struct comedi_device *dev,
 			    struct comedi_insn *insn, unsigned int *data)
 {
 	struct das16cs_private *devpriv = dev->private;
+	int chan = CR_CHAN(insn->chanspec);
+	int range = CR_RANGE(insn->chanspec);
+	int aref = CR_AREF(insn->chanspec);
 	int i;
 	int to;
-	int aref;
-	int range;
-	int chan;
-	static int range_bits[] = { 0x800, 0x000, 0x100, 0x200 };
-
-	chan = CR_CHAN(insn->chanspec);
-	aref = CR_AREF(insn->chanspec);
-	range = CR_RANGE(insn->chanspec);
 
 	outw(chan, dev->iobase + 2);
 
@@ -129,7 +124,22 @@ static int das16cs_ai_rinsn(struct comedi_device *dev,
 	outw(devpriv->status1, dev->iobase + 4);
 
 	devpriv->status2 &= ~0xff00;
-	devpriv->status2 |= range_bits[range];
+	switch (range) {
+	case 0:
+		devpriv->status2 |= 0x800;
+		break;
+	case 1:
+		devpriv->status2 |= 0x000;
+		break;
+	case 2:
+		devpriv->status2 |= 0x100;
+		break;
+	case 3:
+		devpriv->status2 |= 0x200;
+		break;
+	default:
+		return -EINVAL;
+	}
 	outw(devpriv->status2, dev->iobase + 6);
 
 	for (i = 0; i < insn->n; i++) {
@@ -144,7 +154,7 @@ static int das16cs_ai_rinsn(struct comedi_device *dev,
 			dev_dbg(dev->class_dev, "cb_das16_cs: ai timeout\n");
 			return -ETIME;
 		}
-		data[i] = (unsigned short)inw(dev->iobase + 0);
+		data[i] = inw(dev->iobase + 0);
 	}
 
 	return i;
-- 
1.7.11

Re: [PATCH 28/33] staging: comedi: cb_das16_cs: cleanup das16cs_ai_rinsn()

[Ian Abbott]

On 2012-06-26 01:17, H Hartley Sweeten wrote:
> @@ -129,7 +124,22 @@ static int das16cs_ai_rinsn(struct comedi_device *dev,
>   	outw(devpriv->status1, dev->iobase + 4);
>
>   	devpriv->status2 &= ~0xff00;
> -	devpriv->status2 |= range_bits[range];
> +	switch (range) {
> +	case 0:
> +		devpriv->status2 |= 0x800;
> +		break;
> +	case 1:
> +		devpriv->status2 |= 0x000;
> +		break;
> +	case 2:
> +		devpriv->status2 |= 0x100;
> +		break;
> +	case 3:
> +		devpriv->status2 |= 0x200;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

Is that really an improvement?  The 'range' variable value will be in 
range anyway (the comedi core checks beforehand in 
comedi_check_chanlist()), and looking up the constant to OR with 
devpriv->status2 is probably less object code (and certainly less source 
code).

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH 28/33] staging: comedi: cb_das16_cs: cleanup das16cs_ai_rinsn()

[Ian Abbott]

On 2012-06-26 10:09, Ian Abbott wrote:
> On 2012-06-26 01:17, H Hartley Sweeten wrote:
>> @@ -129,7 +124,22 @@ static int das16cs_ai_rinsn(struct comedi_device *dev,
>>    	outw(devpriv->status1, dev->iobase + 4);
>>
>>    	devpriv->status2 &= ~0xff00;
>> -	devpriv->status2 |= range_bits[range];
>> +	switch (range) {
>> +	case 0:
>> +		devpriv->status2 |= 0x800;
>> +		break;
>> +	case 1:
>> +		devpriv->status2 |= 0x000;
>> +		break;
>> +	case 2:
>> +		devpriv->status2 |= 0x100;
>> +		break;
>> +	case 3:
>> +		devpriv->status2 |= 0x200;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>
> Is that really an improvement?  The 'range' variable value will be in
> range anyway (the comedi core checks beforehand in
> comedi_check_chanlist()), and looking up the constant to OR with
> devpriv->status2 is probably less object code (and certainly less source
> code).

I meant looking up the constant in the static array of course.  It 
doesn't really matter if you want to do it this way though, and the 
`static int range_bits[]` you removed should have been `static const int 
range_bits[]` anyway.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

RE: [PATCH 28/33] staging: comedi: cb_das16_cs: cleanup das16cs_ai_rinsn()

[H Hartley Sweeten]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1482 bytes --]

On Tuesday, June 26, 2012 2:16 AM, Ian Abbott wrote:
> On 2012-06-26 10:09, Ian Abbott wrote:
>> On 2012-06-26 01:17, H Hartley Sweeten wrote:
>>> @@ -129,7 +124,22 @@ static int das16cs_ai_rinsn(struct comedi_device *dev,
>>>    	outw(devpriv->status1, dev->iobase + 4);
>>>
>>>    	devpriv->status2 &= ~0xff00;
>>> -	devpriv->status2 |= range_bits[range];
>>> +	switch (range) {
>>> +	case 0:
>>> +		devpriv->status2 |= 0x800;
>>> +		break;
>>> +	case 1:
>>> +		devpriv->status2 |= 0x000;
>>> +		break;
>>> +	case 2:
>>> +		devpriv->status2 |= 0x100;
>>> +		break;
>>> +	case 3:
>>> +		devpriv->status2 |= 0x200;
>>> +		break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>
>> Is that really an improvement?  The 'range' variable value will be in
>> range anyway (the comedi core checks beforehand in
>> comedi_check_chanlist()), and looking up the constant to OR with
>> devpriv->status2 is probably less object code (and certainly less source
>> code).
>
> I meant looking up the constant in the static array of course.  It 
> doesn't really matter if you want to do it this way though, and the 
> `static int range_bits[]` you removed should have been `static const int 
> range_bits[]` anyway.

This, and your other comment about the PCMCIA support, have
been addressed.

Thanks for the review!

Regards,
Hartley

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥