dsbr100: fix codinstyle, make ifs more clear

PATCH In review Alexey Klimov 4 participants 7 messages Dec 20, 2008 – Dec 21, 2008 Media (V4L/DVB)

lore

Share & tools

lore

Alexey Klimov

author

2008-12-20 03:09 UTC #0 lore raw

PATCH 2/5 dsbr100: fix codinstyle, make ifs more clear

We should make if-constructions more clear. Introduce int variables in some functions to make it this way.

---

linux/drivers/media/radio/dsbr100.c +81 ~~-37~~

diff -r a302bfcb23f8 linux/drivers/media/radio/dsbr100.c
@@ -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;
 	}

@@ -222,15 +231,24 @@
 /* switch off radio */
 static int dsbr100_stop(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,
-			0x16, 0x1C, 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,
-			0x00, 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,
+		0x16, 0x1C, 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,
+		0x00, 0x00, radio->transfer_buffer, 8, 300);
+
+	if (first < 0 || second < 0) {
 		mutex_unlock(&radio->lock);
 		return -1;
 	}

@@ -243,21 +261,33 @@
 /* set a frequency, freq is defined by v4l's TUNER_LOW, i.e. 1/16th kHz */
 static int dsbr100_setfreq(struct dsbr100_device *radio, int freq)
 {
+	int first;
+	int second;
+	int third;
+
 	freq = (freq / 16 * 80) / 1000 + 856;
 	mutex_lock(&radio->lock);
-	if (usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
-			DSB100_TUNE,
-			USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
-			(freq >> 8) & 0x00ff, freq & 0xff,
-			radio->transfer_buffer, 8, 300) < 0 ||
-	   usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
-			USB_REQ_GET_STATUS,
-			USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
-			0x96, 0xB7, radio->transfer_buffer, 8, 300) < 0 ||
-	usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
-			USB_REQ_GET_STATUS,
-			USB_TYPE_VENDOR | USB_RECIP_DEVICE |  USB_DIR_IN,
-			0x00, 0x24, radio->transfer_buffer, 8, 300) < 0) {
+
+	first = usb_control_msg(radio->usbdev,
+		usb_rcvctrlpipe(radio->usbdev, 0),
+		DSB100_TUNE,
+		USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+		(freq >> 8) & 0x00ff, freq & 0xff,
+		radio->transfer_buffer, 8, 300);
+
+	second = usb_control_msg(radio->usbdev,
+		usb_rcvctrlpipe(radio->usbdev, 0),
+		USB_REQ_GET_STATUS,
+		USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+		0x96, 0xB7, radio->transfer_buffer, 8, 300);
+
+	third = usb_control_msg(radio->usbdev,
+		usb_rcvctrlpipe(radio->usbdev, 0),
+		USB_REQ_GET_STATUS,
+		USB_TYPE_VENDOR | USB_RECIP_DEVICE |  USB_DIR_IN,
+		0x00, 0x24, radio->transfer_buffer, 8, 300);
+
+	if (first < 0 || second < 0 || third < 0) {
 		radio->stereo = -1;
 		mutex_unlock(&radio->lock);
 		return -1;

@@ -272,14 +302,21 @@
 sees a stereo signal or not.  Pity. */
 static void dsbr100_getstat(struct dsbr100_device *radio)
 {
+	int retval;
+
 	mutex_lock(&radio->lock);
-	if (usb_control_msg(radio->usbdev, usb_rcvctrlpipe(radio->usbdev, 0),
+
+	retval = usb_control_msg(radio->usbdev,
+		usb_rcvctrlpipe(radio->usbdev, 0),
 		USB_REQ_GET_STATUS,
 		USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
-		0x00 , 0x24, radio->transfer_buffer, 8, 300) < 0)
+		0x00 , 0x24, radio->transfer_buffer, 8, 300);
+
+	if (retval < 0)
 		radio->stereo = -1;
 	else
 		radio->stereo = !(radio->transfer_buffer[0] & 0x01);
+
 	mutex_unlock(&radio->lock);
 }

@@ -361,13 +398,15 @@
 				struct v4l2_frequency *f)
 {
 	struct dsbr100_device *radio = video_drvdata(file);
+	int retval;
 
 	/* safety check */
 	if (radio->removed)
 		return -EIO;
 
 	radio->curfreq = f->frequency;
-	if (dsbr100_setfreq(radio, radio->curfreq) == -1)
+	retval = dsbr100_setfreq(radio, radio->curfreq);
+	if (retval == -1)
 		dev_warn(&radio->usbdev->dev, "Set frequency failed\n");
 	return 0;
 }

@@ -421,6 +460,7 @@
 				struct v4l2_control *ctrl)
 {
 	struct dsbr100_device *radio = video_drvdata(file);
+	int retval;
 
 	/* safety check */
 	if (radio->removed)

@@ -429,13 +469,15 @@
 	switch (ctrl->id) {
 	case V4L2_CID_AUDIO_MUTE:
 		if (ctrl->value) {
-			if (dsbr100_stop(radio) == -1) {
+			retval = dsbr100_stop(radio);
+			if (retval == -1) {
 				dev_warn(&radio->usbdev->dev,
 					 "Radio did not respond properly\n");
 				return -EBUSY;
 			}
 		} else {
-			if (dsbr100_start(radio) == -1) {
+			retval = dsbr100_start(radio);
+			if (retval == -1) {
 				dev_warn(&radio->usbdev->dev,
 					 "Radio did not respond properly\n");
 				return -EBUSY;

@@ -487,7 +529,8 @@
 	radio->users = 1;
 	radio->muted = 1;
 
-	if (dsbr100_start(radio) < 0) {
+	retval = dsbr100_start(radio);
+	if (retval < 0) {
 		dev_warn(&radio->usbdev->dev,
 			 "Radio did not start up properly\n");
 		radio->users = 0;

@@ -496,7 +539,6 @@
 	}
 
 	retval = dsbr100_setfreq(radio, radio->curfreq);
-
 	if (retval == -1)
 		dev_warn(&radio->usbdev->dev,
 			"set frequency failed\n");

@@ -604,6 +646,7 @@
 				const struct usb_device_id *id)
 {
 	struct dsbr100_device *radio;
+	int retval;
 
 	radio = kmalloc(sizeof(struct dsbr100_device), GFP_KERNEL);

@@ -625,7 +668,8 @@
 	radio->usbdev = interface_to_usbdev(intf);
 	radio->curfreq = FREQ_MIN * FREQ_MUL;
 	video_set_drvdata(&radio->videodev, radio);
-	if (video_register_device(&radio->videodev, VFL_TYPE_RADIO, radio_nr) < 0) {
+	retval = video_register_device(&radio->videodev, VFL_TYPE_RADIO, radio_nr);
+	if (retval < 0) {
 		dev_warn(&intf->dev, "Could not register video device\n");
 		kfree(radio->transfer_buffer);
 		kfree(radio);

Signature · 6 lines

Best regards, Klimov Alexey

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Douglas Schilling Landgraf

2008-12-20 15:27 UTC #1 lore raw

Hello Alexey,

Quoted 35 lines · On Sat, 20 Dec 2008 06:09:23 +0300 Alexey Klimov <[email protected]> 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) {
linux/drivers/media/radio/dsbr100.c +3 -1
+	int first;
+	int second;
+
 	mutex_lock(&radio->lock);
-	if (usb_control_msg(radio->usbdev,
usb_rcvctrlpipe(radio->usbdev, 0),
(quoted diff) +0 -2
-			USB_REQ_GET_STATUS,
-			USB_TYPE_VENDOR | USB_RECIP_DEVICE |
USB_DIR_IN,
(quoted diff) +0 -1
-			0x00, 0xC7, radio->transfer_buffer, 8, 300)
< 0 ||
(quoted diff) +0 -1
-	usb_control_msg(radio->usbdev,
usb_rcvctrlpipe(radio->usbdev, 0),
(quoted diff) +0 -2
-			DSB100_ONOFF,
-			USB_TYPE_VENDOR | USB_RECIP_DEVICE |
USB_DIR_IN,
(quoted diff) +0 -1
-			0x01, 0x00, radio->transfer_buffer, 8, 300)
< 0) { +
linux/drivers/media/radio/dsbr100.c +6 -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);
+

linux/drivers/media/radio/dsbr100.c line 223

Quoted 10 lines

linux/drivers/media/radio/dsbr100.c +7 -0

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

Quoted 28 lines

linux/drivers/media/radio/dsbr100.c +3 -4

@@ -222,15 +231,24 @@
 /* switch off radio */
 static int dsbr100_stop(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,
-			0x16, 0x1C, radio->transfer_buffer, 8, 300)

< 0 ||

(quoted diff) +0 -1

-	usb_control_msg(radio->usbdev,

usb_rcvctrlpipe(radio->usbdev, 0),

(quoted diff) +0 -2

-			DSB100_ONOFF,
-			USB_TYPE_VENDOR | USB_RECIP_DEVICE |

USB_DIR_IN,

(quoted diff) +0 -1

-			0x00, 0x00, radio->transfer_buffer, 8, 300)

< 0) { +

linux/drivers/media/radio/dsbr100.c +6 -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,
+		0x16, 0x1C, radio->transfer_buffer, 8, 300);
+

linux/drivers/media/radio/dsbr100.c line 254

Quoted 10 lines

linux/drivers/media/radio/dsbr100.c +7 -0

+	second = usb_control_msg(radio->usbdev,
+		usb_rcvctrlpipe(radio->usbdev, 0),
+		DSB100_ONOFF,
+		USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+		0x00, 0x00, radio->transfer_buffer, 8, 300);
+
+	if (first < 0 || second < 0) {
 		mutex_unlock(&radio->lock);
 		return -1;
 	}

The same here.

Quoted 44 lines

(quoted diff) +0 -0

@@ -243,21 +261,33 @@
 /* set a frequency, freq is defined by v4l's TUNER_LOW, i.e. 1/16th

kHz */ static int dsbr100_setfreq(struct dsbr100_device *radio, int freq) {

linux/drivers/media/radio/dsbr100.c +4 -1

+	int first;
+	int second;
+	int third;
+
 	freq = (freq / 16 * 80) / 1000 + 856;
 	mutex_lock(&radio->lock);
-	if (usb_control_msg(radio->usbdev,

usb_rcvctrlpipe(radio->usbdev, 0),

(quoted diff) +0 -2

-			DSB100_TUNE,
-			USB_TYPE_VENDOR | USB_RECIP_DEVICE |

USB_DIR_IN,

linux/drivers/media/radio/dsbr100.c +0 -3

-			(freq >> 8) & 0x00ff, freq & 0xff,
-			radio->transfer_buffer, 8, 300) < 0 ||
-	   usb_control_msg(radio->usbdev,

usb_rcvctrlpipe(radio->usbdev, 0),

(quoted diff) +0 -2

-			USB_REQ_GET_STATUS,
-			USB_TYPE_VENDOR | USB_RECIP_DEVICE |

USB_DIR_IN,

(quoted diff) +0 -1

-			0x96, 0xB7, radio->transfer_buffer, 8, 300)

< 0 ||

(quoted diff) +0 -1

-	usb_control_msg(radio->usbdev,

usb_rcvctrlpipe(radio->usbdev, 0),

(quoted diff) +0 -2

-			USB_REQ_GET_STATUS,
-			USB_TYPE_VENDOR | USB_RECIP_DEVICE |

USB_DIR_IN,

(quoted diff) +0 -1

-			0x00, 0x24, radio->transfer_buffer, 8, 300)

< 0) { +

linux/drivers/media/radio/dsbr100.c +13 -0

+	first = usb_control_msg(radio->usbdev,
+		usb_rcvctrlpipe(radio->usbdev, 0),
+		DSB100_TUNE,
+		USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+		(freq >> 8) & 0x00ff, freq & 0xff,
+		radio->transfer_buffer, 8, 300);
+
+	second = usb_control_msg(radio->usbdev,
+		usb_rcvctrlpipe(radio->usbdev, 0),
+		USB_REQ_GET_STATUS,
+		USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+		0x96, 0xB7, radio->transfer_buffer, 8, 300);
+

linux/drivers/media/radio/dsbr100.c line 293

Quoted 10 lines

linux/drivers/media/radio/dsbr100.c +7 -0

+	third = usb_control_msg(radio->usbdev,
+		usb_rcvctrlpipe(radio->usbdev, 0),
+		USB_REQ_GET_STATUS,
+		USB_TYPE_VENDOR | USB_RECIP_DEVICE |  USB_DIR_IN,
+		0x00, 0x24, radio->transfer_buffer, 8, 300);
+
+	if (first < 0 || second < 0 || third < 0) {
 		radio->stereo = -1;
 		mutex_unlock(&radio->lock);
 		return -1;

The same here. What do you think?

Cheers,
Douglas

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

David Ellingsworth

2008-12-20 17:59 UTC #2 lore raw

Quoted 45 lines · Douglas Schilling Landgraf

Hello Alexey,
Quoted 40 lines · On Sat, 20 Dec 2008 06:09:23 +0300 Alexey Klimov <[email protected]> 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) {
linux/drivers/media/radio/dsbr100.c +3 -1
+     int first;
+     int second;
+
      mutex_lock(&radio->lock);
-     if (usb_control_msg(radio->usbdev,
usb_rcvctrlpipe(radio->usbdev, 0),
(quoted diff) +0 -2
-                     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),
(quoted diff) +0 -2
-                     DSB100_ONOFF,
-                     USB_TYPE_VENDOR | USB_RECIP_DEVICE |
USB_DIR_IN,
- 0x01, 0x00, radio->transfer_buffer, 8, 300)
< 0) { +
linux/drivers/media/radio/dsbr100.c +11 -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);

Quoted 8 lines

linux/drivers/media/radio/dsbr100.c line 223
Quoted 5 lines
linux/drivers/media/radio/dsbr100.c +2 -0
+
+     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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Alexey Klimov

author

2008-12-21 18:46 UTC #3 lore raw

Quoted 62 lines · David Ellingsworth

Quoted 45 lines · Douglas Schilling Landgraf
Hello Alexey,
Quoted 40 lines · On Sat, 20 Dec 2008 06:09:23 +0300 Alexey Klimov <[email protected]> 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) {
linux/drivers/media/radio/dsbr100.c +3 -1
+     int first;
+     int second;
+
      mutex_lock(&radio->lock);
-     if (usb_control_msg(radio->usbdev,
usb_rcvctrlpipe(radio->usbdev, 0),
(quoted diff) +0 -2
-                     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),
(quoted diff) +0 -2
-                     DSB100_ONOFF,
-                     USB_TYPE_VENDOR | USB_RECIP_DEVICE |
USB_DIR_IN,
- 0x01, 0x00, radio->transfer_buffer, 8, 300)
< 0) { +
linux/drivers/media/radio/dsbr100.c +11 -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);
Quoted 8 lines
linux/drivers/media/radio/dsbr100.c line 223
Quoted 5 lines
linux/drivers/media/radio/dsbr100.c +2 -0
+
+     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.

Quoted 8 lines

I agree with Douglas, a single "ret" variable should be used and then evaluated after every usb_control_msg call.
[snip]
Regards,
David Ellingsworth

Hello, all
Yes, you are right, my bad - i missed that thing.

Also, it's better to add dev_err messages that reports in case of retval<0 what usb_control_msg returned, request and what function it was.

I suppose example could look like this:

static int dsbr100_start(struct dsbr100_device *radio)

{
        int retval;

        mutex_lock(&radio->lock);

        retval = 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);

        if (retval < 0) {
                mutex_unlock(&radio->lock);
                dev_err(&radio->usbdev->dev,
                        "usb_control_msg returned %i, request %i in %s\n",
				retval, USB_REQ_GET_STATUS, __func__);
                return -1;
        }

        retval = 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 (retval < 0) {
                mutex_unlock(&radio->lock);
                dev_err(&radio->usbdev->dev,
                        "usb_control_msg returned %i, request %i in %s\n",
				retval, DSB100_ONOFF, __func__);
                return -1;
        }

...<snip>

But it looks, that more comfortable to create macro, may be smth that looks like:

#define dsbr_usb_control_msg_err(arg)                                   \
                dev_err(&radio->usbdev->dev,                    	\
                        "%s - usb_control_msg returned %i, request %i\n",\
                                __func__, retval, arg)

And then i can use:

        retval = 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);

        if (retval < 0) {
                mutex_unlock(&radio->lock);
                dsbr_usb_control_msg_err(USB_REQ_GET_STATUS);
                return -1;
        }

        retval = 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 (retval < 0) {
                mutex_unlock(&radio->lock);
                dsbr_usb_control_msg_err(DSB100_ONOFF);
                return -1;
        }

We should see what function and request failed.
I didn't mean that this is right thing, it's just approach, that i can offer.

What do you think ?

Signature · 6 lines

Best regards, Klimov Alexey

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Thierry Merle

2008-12-21 20:14 UTC #4 lore raw

Hello,

Quoted 147 lines · Alexey Klimov

Quoted 60 lines · David Ellingsworth
Quoted 44 lines · Douglas Schilling Landgraf
Hello Alexey,
Quoted 39 lines · On Sat, 20 Dec 2008 06:09:23 +0300 Alexey Klimov <[email protected]> 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) {
linux/drivers/media/radio/dsbr100.c +3 -1
+     int first;
+     int second;
+
      mutex_lock(&radio->lock);
-     if (usb_control_msg(radio->usbdev,
usb_rcvctrlpipe(radio->usbdev, 0),
(quoted diff) +0 -2
-                     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),
(quoted diff) +0 -2
-                     DSB100_ONOFF,
-                     USB_TYPE_VENDOR | USB_RECIP_DEVICE |
USB_DIR_IN,
- 0x01, 0x00, radio->transfer_buffer, 8, 300)
< 0) { +
linux/drivers/media/radio/dsbr100.c +10 -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,
Quoted 8 lines
linux/drivers/media/radio/dsbr100.c line 223
Quoted 6 lines
linux/drivers/media/radio/dsbr100.c +3 -0
+             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.
Quoted 8 lines
I agree with Douglas, a single "ret" variable should be used and then evaluated after every usb_control_msg call.
[snip]
Regards,
David Ellingsworth
Hello, all
Yes, you are right, my bad - i missed that thing.
Also, it's better to add dev_err messages that reports in case of retval<0 what usb_control_msg returned, request and what function it was.
I suppose example could look like this:
static int dsbr100_start(struct dsbr100_device *radio)
{
        int retval;

        mutex_lock(&radio->lock);

        retval = 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);

        if (retval < 0) {
                mutex_unlock(&radio->lock);
                dev_err(&radio->usbdev->dev,
                        "usb_control_msg returned %i, request %i in %s\n",
				retval, USB_REQ_GET_STATUS, __func__);
                return -1;
        }

        retval = 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 (retval < 0) {
                mutex_unlock(&radio->lock);
                dev_err(&radio->usbdev->dev,
                        "usb_control_msg returned %i, request %i in %s\n",
				retval, DSB100_ONOFF, __func__);
                return -1;
        }
...<snip>
But it looks, that more comfortable to create macro, may be smth that looks like:
#define dsbr_usb_control_msg_err(arg)                                   \
                dev_err(&radio->usbdev->dev,                    	\
                        "%s - usb_control_msg returned %i, request %i\n",\
                                __func__, retval, arg)
And then i can use:
        retval = 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);

        if (retval < 0) {
                mutex_unlock(&radio->lock);
                dsbr_usb_control_msg_err(USB_REQ_GET_STATUS);
                return -1;
        }

        retval = 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 (retval < 0) {
                mutex_unlock(&radio->lock);
                dsbr_usb_control_msg_err(DSB100_ONOFF);
                return -1;
        }

Quoted 7 lines

We should see what function and request failed.
I didn't mean that this is right thing, it's just approach, that i can offer.
What do you think ?

I would use a goto.
This is the most readable and efficient way to manage exeptions in C. Take a look at linux/drivers/media/video/v4l2-dev.c for an example of goto usage. Cheers,
Thierry

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Alexey Klimov

author

2008-12-21 20:52 UTC #5 lore raw

Quoted 154 lines · Thierry Merle

Hello,
Quoted 151 lines · Alexey Klimov
Quoted 60 lines · David Ellingsworth
Quoted 44 lines · Douglas Schilling Landgraf
Hello Alexey,
Quoted 39 lines · On Sat, 20 Dec 2008 06:09:23 +0300 Alexey Klimov <[email protected]> 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) {
linux/drivers/media/radio/dsbr100.c +3 -1
+     int first;
+     int second;
+
      mutex_lock(&radio->lock);
-     if (usb_control_msg(radio->usbdev,
usb_rcvctrlpipe(radio->usbdev, 0),
(quoted diff) +0 -2
-                     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),
(quoted diff) +0 -2
-                     DSB100_ONOFF,
-                     USB_TYPE_VENDOR | USB_RECIP_DEVICE |
USB_DIR_IN,
- 0x01, 0x00, radio->transfer_buffer, 8, 300)
< 0) { +
linux/drivers/media/radio/dsbr100.c +10 -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,
Quoted 8 lines
linux/drivers/media/radio/dsbr100.c line 223
Quoted 6 lines
linux/drivers/media/radio/dsbr100.c +3 -0
+             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.
Quoted 8 lines
I agree with Douglas, a single "ret" variable should be used and then evaluated after every usb_control_msg call.
[snip]
Regards,
David Ellingsworth
Hello, all
Yes, you are right, my bad - i missed that thing.
Also, it's better to add dev_err messages that reports in case of retval<0 what usb_control_msg returned, request and what function it was.
I suppose example could look like this:
static int dsbr100_start(struct dsbr100_device *radio)
{
        int retval;

        mutex_lock(&radio->lock);

        retval = 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);

        if (retval < 0) {
                mutex_unlock(&radio->lock);
                dev_err(&radio->usbdev->dev,
                        "usb_control_msg returned %i, request %i in %s\n",
                              retval, USB_REQ_GET_STATUS, __func__);
                return -1;
        }

        retval = 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 (retval < 0) {
                mutex_unlock(&radio->lock);
                dev_err(&radio->usbdev->dev,
                        "usb_control_msg returned %i, request %i in %s\n",
                              retval, DSB100_ONOFF, __func__);
                return -1;
        }
...<snip>
But it looks, that more comfortable to create macro, may be smth that looks like:
#define dsbr_usb_control_msg_err(arg)                                   \
                dev_err(&radio->usbdev->dev,                          \
                        "%s - usb_control_msg returned %i, request %i\n",\
                                __func__, retval, arg)
And then i can use:
        retval = 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);

        if (retval < 0) {
                mutex_unlock(&radio->lock);
                dsbr_usb_control_msg_err(USB_REQ_GET_STATUS);
                return -1;
        }

        retval = 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 (retval < 0) {
                mutex_unlock(&radio->lock);
                dsbr_usb_control_msg_err(DSB100_ONOFF);
                return -1;
        }
We should see what function and request failed.
I didn't mean that this is right thing, it's just approach, that i can offer.

Quoted 8 lines

Quoted 3 lines
What do you think ?
I would use a goto.
This is the most readable and efficient way to manage exeptions in C. Take a look at linux/drivers/media/video/v4l2-dev.c for an example of goto usage. Cheers,
Thierry

So, do you mean that i can use this method ?

static int dsbr100_stop(struct dsbr100_device *radio)

{
        int retval;
        int request;

        mutex_lock(&radio->lock);

        retval = usb_control_msg(radio->usbdev,
                usb_rcvctrlpipe(radio->usbdev, 0),
                USB_REQ_GET_STATUS,
                USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
                0x16, 0x1C, radio->transfer_buffer, 8, 300);

        if (retval < 0) {
                request = USB_REQ_GET_STATUS;
                goto usb_control_msg_failed;
        }

        retval = usb_control_msg(radio->usbdev,
                usb_rcvctrlpipe(radio->usbdev, 0),
                DSB100_ONOFF,
                USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
                0x00, 0x00, radio->transfer_buffer, 8, 300);

        if (retval < 0) {
                request = DSB100_ONOFF;
                goto usb_control_msg_failed;
        }

        radio->muted=1;
        mutex_unlock(&radio->lock);
        return (radio->transfer_buffer)[0];

:usb_control_msg_failed:

        mutex_unlock(&radio->lock);
        dev_err(&radio->usbdev->dev,
                "%s - usb_control_msg returned %i, request %i\n",
                        __func__, retval, request);
        return -1;
}

And i have to make it in three functions ?

Signature · 6 lines

Best regards, Klimov Alexey

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Alexey Klimov

author

2008-12-21 21:00 UTC #6 lore raw

<snip>

Quoted 41 lines

Quoted 5 lines

I would use a goto.
This is the most readable and efficient way to manage exeptions in C. Take a look at linux/drivers/media/video/v4l2-dev.c for an example of goto usage. Cheers,
Thierry

So, do you mean that i can use this method ?

static int dsbr100_stop(struct dsbr100_device *radio)

{
       int retval;
       int request;

       mutex_lock(&radio->lock);

       retval = usb_control_msg(radio->usbdev,
               usb_rcvctrlpipe(radio->usbdev, 0),
               USB_REQ_GET_STATUS,
               USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
               0x16, 0x1C, radio->transfer_buffer, 8, 300);

       if (retval < 0) {
               request = USB_REQ_GET_STATUS;
               goto usb_control_msg_failed;
       }

       retval = usb_control_msg(radio->usbdev,
               usb_rcvctrlpipe(radio->usbdev, 0),
               DSB100_ONOFF,
               USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
               0x00, 0x00, radio->transfer_buffer, 8, 300);

       if (retval < 0) {
               request = DSB100_ONOFF;
               goto usb_control_msg_failed;
       }

       radio->muted=1;
       mutex_unlock(&radio->lock);
       return (radio->transfer_buffer)[0];

Quoted 1 line

:usb_control_msg_failed:

Oh, sorry. Of course, there is no ":" before usb_control_msg_failed. Only after.

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list