* [review patch 2/5] dsbr100: fix codinstyle, make ifs more clear
@ 2008-12-20 3:09 Alexey Klimov
2008-12-20 15:27 ` Douglas Schilling Landgraf
0 siblings, 1 reply; 7+ messages in thread
From: Alexey Klimov @ 2008-12-20 3:09 UTC (permalink / raw)
To: Douglas Schilling Landgraf; +Cc: video4linux-list
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;
}
@@ -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);
--
Best regards, Klimov Alexey
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [review patch 2/5] dsbr100: fix codinstyle, make ifs more clear
2008-12-20 3:09 [review patch 2/5] dsbr100: fix codinstyle, make ifs more clear Alexey Klimov
@ 2008-12-20 15:27 ` Douglas Schilling Landgraf
2008-12-20 17:59 ` David Ellingsworth
0 siblings, 1 reply; 7+ messages in thread
From: Douglas Schilling Landgraf @ 2008-12-20 15:27 UTC (permalink / raw)
To: Alexey Klimov; +Cc: video4linux-list
Hello Alexey,
On Sat, 20 Dec 2008 06:09:23 +0300
Alexey Klimov <klimov.linux@gmail.com> 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".
> @@ -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;
> }
The same here.
> @@ -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;
The same here. What do you think?
Cheers,
Douglas
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [review patch 2/5] dsbr100: fix codinstyle, make ifs more clear
2008-12-20 15:27 ` Douglas Schilling Landgraf
@ 2008-12-20 17:59 ` David Ellingsworth
2008-12-21 18:46 ` Alexey Klimov
0 siblings, 1 reply; 7+ messages in thread
From: David Ellingsworth @ 2008-12-20 17:59 UTC (permalink / raw)
To: Douglas Schilling Landgraf; +Cc: video4linux-list
On Sat, Dec 20, 2008 at 10:27 AM, Douglas Schilling Landgraf
<dougsland@gmail.com> wrote:
> Hello Alexey,
>
> On Sat, 20 Dec 2008 06:09:23 +0300
> Alexey Klimov <klimov.linux@gmail.com> 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@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [review patch 2/5] dsbr100: fix codinstyle, make ifs more clear
2008-12-20 17:59 ` David Ellingsworth
@ 2008-12-21 18:46 ` Alexey Klimov
2008-12-21 20:13 ` Thierry Merle
0 siblings, 1 reply; 7+ messages in thread
From: Alexey Klimov @ 2008-12-21 18:46 UTC (permalink / raw)
To: David Ellingsworth; +Cc: video4linux-list
On Sat, 2008-12-20 at 12:59 -0500, David Ellingsworth wrote:
> On Sat, Dec 20, 2008 at 10:27 AM, Douglas Schilling Landgraf
> <dougsland@gmail.com> wrote:
> > Hello Alexey,
> >
> > On Sat, 20 Dec 2008 06:09:23 +0300
> > Alexey Klimov <klimov.linux@gmail.com> 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
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 ?
--
Best regards, Klimov Alexey
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [review patch 2/5] dsbr100: fix codinstyle, make ifs more clear
2008-12-21 18:46 ` Alexey Klimov
@ 2008-12-21 20:13 ` Thierry Merle
2008-12-21 20:51 ` Alexey Klimov
0 siblings, 1 reply; 7+ messages in thread
From: Thierry Merle @ 2008-12-21 20:13 UTC (permalink / raw)
To: Alexey Klimov; +Cc: video4linux-list, David Ellingsworth
Hello,
Alexey Klimov a écrit :
> On Sat, 2008-12-20 at 12:59 -0500, David Ellingsworth wrote:
>> On Sat, Dec 20, 2008 at 10:27 AM, Douglas Schilling Landgraf
>> <dougsland@gmail.com> wrote:
>>> Hello Alexey,
>>>
>>> On Sat, 20 Dec 2008 06:09:23 +0300
>>> Alexey Klimov <klimov.linux@gmail.com> 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
>
> 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 ?
>
>
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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [review patch 2/5] dsbr100: fix codinstyle, make ifs more clear
2008-12-21 20:13 ` Thierry Merle
@ 2008-12-21 20:51 ` Alexey Klimov
2008-12-21 21:00 ` Alexey Klimov
0 siblings, 1 reply; 7+ messages in thread
From: Alexey Klimov @ 2008-12-21 20:51 UTC (permalink / raw)
To: Thierry Merle; +Cc: video4linux-list, David Ellingsworth
On Sun, Dec 21, 2008 at 11:13 PM, Thierry Merle <thierry.merle@free.fr> wrote:
> Hello,
>
> Alexey Klimov a écrit :
>> On Sat, 2008-12-20 at 12:59 -0500, David Ellingsworth wrote:
>>> On Sat, Dec 20, 2008 at 10:27 AM, Douglas Schilling Landgraf
>>> <dougsland@gmail.com> wrote:
>>>> Hello Alexey,
>>>>
>>>> On Sat, 20 Dec 2008 06:09:23 +0300
>>>> Alexey Klimov <klimov.linux@gmail.com> 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
>>
>> 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 ?
>>
>>
> 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 ?
--
Best regards, Klimov Alexey
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [review patch 2/5] dsbr100: fix codinstyle, make ifs more clear
2008-12-21 20:51 ` Alexey Klimov
@ 2008-12-21 21:00 ` Alexey Klimov
0 siblings, 0 replies; 7+ messages in thread
From: Alexey Klimov @ 2008-12-21 21:00 UTC (permalink / raw)
To: Thierry Merle; +Cc: video4linux-list, David Ellingsworth
<snip>
>> 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:
Oh, sorry. Of course, there is no ":" before usb_control_msg_failed.
Only after.
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-12-21 21:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-12-20 3:09 [review patch 2/5] dsbr100: fix codinstyle, make ifs more clear Alexey Klimov
2008-12-20 15:27 ` Douglas Schilling Landgraf
2008-12-20 17:59 ` David Ellingsworth
2008-12-21 18:46 ` Alexey Klimov
2008-12-21 20:13 ` Thierry Merle
2008-12-21 20:51 ` Alexey Klimov
2008-12-21 21:00 ` Alexey Klimov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox