RE: [PATCH] comedi: Change error return code for if statement in the function,cb_pcimdas_ai_rinsn

RE: [PATCH] comedi: Change error return code for if statement in the function,cb_pcimdas_ai_rinsn

[Hartley Sweeten]

On Tuesday, February 24, 2015 9:13 PM, Nicholas Krause wrote:
> This changes us using the incorrect error,-ETIMEOUT when checking if
> the channel we are allocating to on the device structure pointer passed 
> to this function is greater then the maximum available channels for this 
> device to the correct error for a channel being out of range,-ECHRNG.
>
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/staging/comedi/drivers/cb_pcimdas.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/cb_pcimdas.c b/drivers/staging/comedi/drivers/cb_pcimdas.c
> index 70dd2c9..d91a6f3 100644
> --- a/drivers/staging/comedi/drivers/cb_pcimdas.c
> +++ b/drivers/staging/comedi/drivers/cb_pcimdas.c
> @@ -121,7 +121,7 @@ static int cb_pcimdas_ai_rinsn(struct comedi_device *dev,
>  		maxchans = s->n_chan;
>  
>  	if (chan > (maxchans - 1))
> -		return -ETIMEDOUT;	/* *** Wrong error code. Fixme. */
> +		return -ECHRNG;
>  
>  	/* configure for sw initiated read */
>  	d = inb(devpriv->BADR3 + 5);

Hmm... This isn't quite right...

The 16 single-ended / 8 differential analog input channels on this board is
set with a switch on the PCB. The state of the switch should be read when
the driver is attached and the subdevice initialized with the correct number
of channels. The core will then validate the "chan" number before calling
the (*insn_read) operation.

Regards,
Hartley

Re: [PATCH] comedi: Change error return code for if statement in the function,cb_pcimdas_ai_rinsn

[Ian Abbott]

On 25/02/15 21:37, Nicholas Krause wrote:
>
>
> On February 25, 2015 1:03:14 PM EST, Hartley Sweeten <HartleyS@visionengravers.com> wrote:
>> On Tuesday, February 24, 2015 9:13 PM, Nicholas Krause wrote:
>>> This changes us using the incorrect error,-ETIMEOUT when checking if
>>> the channel we are allocating to on the device structure pointer
>> passed
>>> to this function is greater then the maximum available channels for
>> this
>>> device to the correct error for a channel being out of range,-ECHRNG.
>>>
>>> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>>> ---
>>>   drivers/staging/comedi/drivers/cb_pcimdas.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/comedi/drivers/cb_pcimdas.c
>> b/drivers/staging/comedi/drivers/cb_pcimdas.c
>>> index 70dd2c9..d91a6f3 100644
>>> --- a/drivers/staging/comedi/drivers/cb_pcimdas.c
>>> +++ b/drivers/staging/comedi/drivers/cb_pcimdas.c
>>> @@ -121,7 +121,7 @@ static int cb_pcimdas_ai_rinsn(struct
>> comedi_device *dev,
>>>   		maxchans = s->n_chan;
>>>
>>>   	if (chan > (maxchans - 1))
>>> -		return -ETIMEDOUT;	/* *** Wrong error code. Fixme. */
>>> +		return -ECHRNG;
>>>
>>>   	/* configure for sw initiated read */
>>>   	d = inb(devpriv->BADR3 + 5);
>>
>> Hmm... This isn't quite right...
>>
>> The 16 single-ended / 8 differential analog input channels on this
>> board is
>> set with a switch on the PCB. The state of the switch should be read
>> when
>> the driver is attached and the subdevice initialized with the correct
>> number
>> of channels. The core will then validate the "chan" number before
>> calling
>> the (*insn_read) operation.
>>
>> Regards,
>> Hartley
> Hartley,
> If that is the case then why is the check  for max channels here.  I  can send in a v2 removing this check as it seems unneeded based on my understanding and your response to this patch.
> Nick
>

The patch is fine, but now superceded by Hartley's series of patches 
which has more extensive changes to the driver:

http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2015-February/065754.html

That set of patches assumes the switches remain in the same state 
through the lifetime of the device instance, which seems reasonable for 
a PCI card.

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