Skip to contentSkip to conversation

cx25821: testing the wrong variable

PATCH In review Dan Carpenter 5 participants 6 messages Sep 29, 2012 – Oct 8, 2012

Subsystems (2)
Media (V4L/DVB) Kernel janitors
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 cx25821: testing the wrong variable

->input_filename could be NULL here. The intent was to test ->_filename.

Signed-off-by Dan Carpenter <[email protected]>

---
I'm not totally convinced that using /root/vid411.yuv is the right idea.

drivers/media/pci/cx25821/cx25821-video-upstream.c +1 -1
index 52c13e0..6759fff 100644
@@ -808,7 +808,7 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 	}
 
 	/* Default if filename is empty string */
-	if (strcmp(dev->input_filename, "") = 0) {
+	if (strcmp(dev->_filename, "") = 0) {
 		if (dev->_isNTSC) {
 			dev->_filename  				(dev->_pixel_format = PIXEL_FRMT_411) ?
drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c +1 -1
index c8c94fb..d33fc1a 100644
@@ -761,7 +761,7 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
 	}
 
 	/* Default if filename is empty string */
-	if (strcmp(dev->input_filename_ch2, "") = 0) {
+	if (strcmp(dev->_filename_ch2, "") = 0) {
 		if (dev->_isNTSC_ch2) {
 			dev->_filename_ch2 = (dev->_pixel_format_ch2 =
 				PIXEL_FRMT_411) ? "/root/vid411.yuv" :
#1 lore raw

Am 29.09.2012 09:12, schrieb Dan Carpenter:

Quoted 25 lines

->input_filename could be NULL here. The intent was to test ->_filename.

Signed-off-by Dan Carpenter <[email protected]>

---
I'm not totally convinced that using /root/vid411.yuv is the right idea.

drivers/media/pci/cx25821/cx25821-video-upstream.c +1 -1
index 52c13e0..6759fff 100644
@@ -808,7 +808,7 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 	}
 
 	/* Default if filename is empty string */
-	if (strcmp(dev->input_filename, "") = 0) {
+	if (strcmp(dev->_filename, "") = 0) {
 		if (dev->_isNTSC) {
 			dev->_filename >  				(dev->_pixel_format = PIXEL_FRMT_411) ?
drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c +0 -0
index c8c94fb..d33fc1a 100644

drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select, · line 767

Quoted 8 lines
drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c +1 -1
@@ -761,7 +761,7 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
 	}
 
 	/* Default if filename is empty string */
-	if (strcmp(dev->input_filename_ch2, "") = 0) {
+	if (strcmp(dev->_filename_ch2, "") = 0) {
 		if (dev->_isNTSC_ch2) {
 			dev->_filename_ch2 = (dev->_pixel_format_ch2 =
 				PIXEL_FRMT_411) ? "/root/vid411.yuv" :

In this case stcmp seems a bit of a overkill. A simple *(dev->_filename_ch2) = 0
should be ok ?

re,
wh

author
#2 lore raw
Quoted 29 lines · walter harms

Am 29.09.2012 09:12, schrieb Dan Carpenter:

Quoted 26 lines

->input_filename could be NULL here. The intent was to test ->_filename.

Signed-off-by Dan Carpenter <[email protected]>

---
I'm not totally convinced that using /root/vid411.yuv is the right idea.

drivers/media/pci/cx25821/cx25821-video-upstream.c +1 -1
index 52c13e0..6759fff 100644
@@ -808,7 +808,7 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 	}
 
 	/* Default if filename is empty string */
-	if (strcmp(dev->input_filename, "") = 0) {
+	if (strcmp(dev->_filename, "") = 0) {
 		if (dev->_isNTSC) {
 			dev->_filename > >  				(dev->_pixel_format = PIXEL_FRMT_411) ?
drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c +0 -0
index c8c94fb..d33fc1a 100644
@@ -761,7 +761,7 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
 	}
Quoted 11 lines

drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select, · line 767

Quoted 7 lines
drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c +1 -1
 	/* Default if filename is empty string */
-	if (strcmp(dev->input_filename_ch2, "") = 0) {
+	if (strcmp(dev->_filename_ch2, "") = 0) {
 		if (dev->_isNTSC_ch2) {
 			dev->_filename_ch2 = (dev->_pixel_format_ch2 =
 				PIXEL_FRMT_411) ? "/root/vid411.yuv" :

In this case stcmp seems a bit of a overkill. A simple *(dev->_filename_ch2) = 0
should be ok ?

I prefer strcmp() actually. More readable.

regards,
dan carpenter

Quoted 7 lines

re,
wh
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 42 lines · Dan Carpenter
Quoted 29 lines · walter harms

Am 29.09.2012 09:12, schrieb Dan Carpenter:

Quoted 26 lines

->input_filename could be NULL here. The intent was to test ->_filename.

Signed-off-by Dan Carpenter <[email protected]>

---
I'm not totally convinced that using /root/vid411.yuv is the right idea.

drivers/media/pci/cx25821/cx25821-video-upstream.c +1 -1
index 52c13e0..6759fff 100644
@@ -808,7 +808,7 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
    }
 
    /* Default if filename is empty string */
-   if (strcmp(dev->input_filename, "") = 0) {
+   if (strcmp(dev->_filename, "") = 0) {
            if (dev->_isNTSC) {
                    dev->_filename >> >                             (dev->_pixel_format = PIXEL_FRMT_411) ?
drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c +0 -0
index c8c94fb..d33fc1a 100644
@@ -761,7 +761,7 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
    }
Quoted 11 lines

drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select, · line 767

Quoted 7 lines
drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c +1 -1
    /* Default if filename is empty string */
-   if (strcmp(dev->input_filename_ch2, "") = 0) {
+   if (strcmp(dev->_filename_ch2, "") = 0) {
            if (dev->_isNTSC_ch2) {
                    dev->_filename_ch2 = (dev->_pixel_format_ch2 =
                            PIXEL_FRMT_411) ? "/root/vid411.yuv" :

In this case stcmp seems a bit of a overkill. A simple *(dev->_filename_ch2) = 0
should be ok ?

Quoted 2 lines

I prefer strcmp() actually. More readable.

... and by no means an overkill.
First, it's not a hotpath or a core component. So readability is the priority. Second, strcmp over empty string should be very very cheap (right?). In some arches it's even some asm code.

"Premature optimization is the root of all evil".

Regards,
Ezequiel.

#4 lore raw
In reply to Dan Carpenter

Em Sat, 29 Sep 2012 10:12:53 +0300
Dan Carpenter <[email protected]> escreveu:

Quoted 6 lines

->input_filename could be NULL here. The intent was to test ->_filename.

Signed-off-by Dan Carpenter <[email protected]>

---
I'm not totally convinced that using /root/vid411.yuv is the right idea.

Agreed.

Palash, Sri,

Why do we need those files?

Regards,
Mauro

Quoted 27 lines
drivers/media/pci/cx25821/cx25821-video-upstream.c +1 -1
index 52c13e0..6759fff 100644
@@ -808,7 +808,7 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 	}
 
 	/* Default if filename is empty string */
-	if (strcmp(dev->input_filename, "") = 0) {
+	if (strcmp(dev->_filename, "") = 0) {
 		if (dev->_isNTSC) {
 			dev->_filename >  				(dev->_pixel_format = PIXEL_FRMT_411) ?
drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c +1 -2
index c8c94fb..d33fc1a 100644
@@ -761,7 +761,7 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
 	}
 
 	/* Default if filename is empty string */
-	if (strcmp(dev->input_filename_ch2, "") = 0) {
+	if (strcmp(dev->_filename_ch2, "") = 0) {
 		if (dev->_isNTSC_ch2) {
 			dev->_filename_ch2 = (dev->_pixel_format_ch2 =
 				PIXEL_FRMT_411) ? "/root/vid411.yuv" :
--
Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Cheers,
Mauro

#5 lore raw

Mauro,

This is to be able to upstream data from the host to the device to be pushed out. I agree this is not an optimal way of doing things and had discussed with Hans on this last month.

I will be making changes to the entire upstream path and get rid of this mechanism that we have in place here.

Rgds,
Palash

-----Original Message-----
From: Mauro Carvalho Chehab [mailto:[email protected]]
Sent: Sunday, October 07, 2012 5:50 AM
To: Palash Bandyopadhyay; Sri Deevi
Cc: Dan Carpenter; Leonid V. Fedorenchik; Thomas Meyer; Hans Verkuil; [email protected]; [email protected]
Subject: Re: [patch] [media] cx25821: testing the wrong variable

Em Sat, 29 Sep 2012 10:12:53 +0300
Dan Carpenter <[email protected]> escreveu:

Quoted 6 lines

->input_filename could be NULL here. The intent was to test ->_filename.

Signed-off-by Dan Carpenter <[email protected]>

---
I'm not totally convinced that using /root/vid411.yuv is the right idea.

Agreed.

Palash, Sri,

Why do we need those files?

Regards,
Mauro

Quoted 29 lines
drivers/media/pci/cx25821/cx25821-video-upstream.c +1 -1
index 52c13e0..6759fff 100644
@@ -808,7 +808,7 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, int channel_select,
 	}
 
 	/* Default if filename is empty string */
-	if (strcmp(dev->input_filename, "") = 0) {
+	if (strcmp(dev->_filename, "") = 0) {
 		if (dev->_isNTSC) {
 			dev->_filename >  				(dev->_pixel_format = PIXEL_FRMT_411) ?
drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c +1 -2
index c8c94fb..d33fc1a 100644
@@ -761,7 +761,7 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, int channel_select,
 	}
 
 	/* Default if filename is empty string */
-	if (strcmp(dev->input_filename_ch2, "") = 0) {
+	if (strcmp(dev->_filename_ch2, "") = 0) {
 		if (dev->_isNTSC_ch2) {
 			dev->_filename_ch2 = (dev->_pixel_format_ch2 =
 				PIXEL_FRMT_411) ? "/root/vid411.yuv" :
--
Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Cheers,
Mauro

Conexant E-mail Firewall (Conexant.Com) made the following annotations
---------------------------------------------------------------------
********************** Legal Disclaimer ****************************

"This email may contain confidential and privileged material for the sole use of the intended recipient. Any unauthorized review, use or distribution by others is strictly prohibited. If you have received the message in error, please advise the sender by reply email and delete the message. Thank you."

**********************************************************************

---------------------------------------------------------------------