Re: [review patch 2/5] dsbr100: fix codinstyle, make ifs more clear

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On Sat, Dec 20, 2008 at 10:27 AM, Douglas Schilling Landgraf
<dougsland@xxxxxxxxx> wrote:
> Hello Alexey,
>
> On Sat, 20 Dec 2008 06:09:23 +0300
> Alexey Klimov <klimov.linux@xxxxxxxxx> wrote:
>
>> We should make if-constructions more clear. Introduce int variables in
>> some functions to make it this way.
>>
>> ---
>> diff -r a302bfcb23f8 linux/drivers/media/radio/dsbr100.c
>> --- a/linux/drivers/media/radio/dsbr100.c     Fri Dec 19 14:34:30
>> 2008 +0300 +++ b/linux/drivers/media/radio/dsbr100.c  Sat Dec
>> 20 02:31:26 2008 +0300 @@ -200,15 +200,24 @@
>>  /* switch on radio */
>>  static int dsbr100_start(struct dsbr100_device *radio)
>>  {
>> +     int first;
>> +     int second;
>> +
>>       mutex_lock(&radio->lock);
>> -     if (usb_control_msg(radio->usbdev,
>> usb_rcvctrlpipe(radio->usbdev, 0),
>> -                     USB_REQ_GET_STATUS,
>> -                     USB_TYPE_VENDOR | USB_RECIP_DEVICE |
>> USB_DIR_IN,
>> -                     0x00, 0xC7, radio->transfer_buffer, 8, 300)
>> < 0 ||
>> -     usb_control_msg(radio->usbdev,
>> usb_rcvctrlpipe(radio->usbdev, 0),
>> -                     DSB100_ONOFF,
>> -                     USB_TYPE_VENDOR | USB_RECIP_DEVICE |
>> USB_DIR_IN,
>> -                     0x01, 0x00, radio->transfer_buffer, 8, 300)
>> < 0) { +
>> +     first = usb_control_msg(radio->usbdev,
>> +             usb_rcvctrlpipe(radio->usbdev, 0),
>> +             USB_REQ_GET_STATUS,
>> +             USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
>> +             0x00, 0xC7, radio->transfer_buffer, 8, 300);
>> +
>> +     second = usb_control_msg(radio->usbdev,
>> +             usb_rcvctrlpipe(radio->usbdev, 0),
>> +             DSB100_ONOFF,
>> +             USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
>> +             0x01, 0x00, radio->transfer_buffer, 8, 300);
>> +
>> +     if (first < 0 || second < 0) {
>>               mutex_unlock(&radio->lock);
>>               return -1;
>>       }
>
> IMO, we could create a variable like "ret" or "retval" to validate each
> usb_control_msg call instead of create 3 variables "first", "second" and "third".

The primary problem I have with this patch is that it changes the
behavior of the driver. The original way it was written the function
would immediately return if one of the calls to usb_control_msg
failed. With this patch, if the first call fails it will still make a
second call to usb_control_msg.

I agree with Douglas, a single "ret" variable should be used and then
evaluated after every usb_control_msg call.

[snip]

Regards,

David Ellingsworth

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@xxxxxxxxxx?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[Linux Media]     [Older V4L]     [Linux DVB]     [Video Disk Recorder]     [Linux Kernel]     [Asterisk]     [Photo]     [DCCP]     [Netdev]     [Xorg]     [Util Linux NG]     [Xfree86]     [Free Photo Albums]     [Fedora Users]     [Fedora Women]     [ALSA Users]     [ALSA Devel]     [SSH]     [DVB Maintainers]     [Linux USB]     [Yosemite Information]

Add to Google Powered by Linux