Skip to contentSkip to conversation

radio-mr800: remove warn- and err- messages

PATCH In review Alexey Klimov 4 participants 19 messages Oct 16, 2008 – Nov 4, 2008 1 message not in the archive Media (V4L/DVB)

lore
Share & tools
lore

Alexey Klimov

author
#0 lore raw

PATCH 1/1 radio-mr800: remove warn- and err- messages

Hello, all

radio-mr800: remove warn and err messages

Patch removes warn() and err()-statements in radio/radio-mr800.c, because of removing this macros from USB-subsystem.

Signature · 1 line
Best regards, Klimov Alexey
linux/drivers/media/radio/radio-mr800.c +15 -9
diff -r 270723a73207 linux/drivers/media/radio/radio-mr800.c
@@ -362,7 +362,7 @@
 
 	radio->curfreq = f->frequency;
 	if (amradio_setfreq(radio, radio->curfreq) < 0)
-		warn("Set frequency failed");
+		printk(KERN_WARNING KBUILD_MODNAME ": Set frequency failed\n");
 	return 0;
 }
 

@@ -417,12 +417,14 @@
 	case V4L2_CID_AUDIO_MUTE:
 		if (ctrl->value) {
 			if (amradio_stop(radio) < 0) {
-				warn("amradio_stop() failed");
+				printk(KERN_WARNING KBUILD_MODNAME
+						": amradio_stop() failed\n");
 				return -1;
 			}
 		} else {
 			if (amradio_start(radio) < 0) {
-				warn("amradio_start() failed");
+				printk(KERN_WARNING KBUILD_MODNAME
+						": amradio_start() failed\n");
 				return -1;
 			}
 		}

@@ -476,12 +478,13 @@
 	radio->muted = 1;
 
 	if (amradio_start(radio) < 0) {
-		warn("Radio did not start up properly");
+		printk(KERN_WARNING KBUILD_MODNAME
+					": Radio did not start up properly\n");
 		radio->users = 0;
 		return -EIO;
 	}
 	if (amradio_setfreq(radio, radio->curfreq) < 0)
-		warn("Set frequency failed");
+		printk(KERN_WARNING KBUILD_MODNAME ": Set frequency failed\n");
 	return 0;
 }
 

@@ -506,7 +509,7 @@
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
 	if (amradio_stop(radio) < 0)
-		warn("amradio_stop() failed");
+		printk(KERN_WARNING KBUILD_MODNAME ": amradio_stop() failed\n");
 
 	printk(KERN_INFO KBUILD_MODNAME ": Going into suspend..\n");
 

@@ -519,7 +522,8 @@
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
 	if (amradio_start(radio) < 0)
-		warn("amradio_start() failed");
+		printk(KERN_WARNING KBUILD_MODNAME
+						": amradio_start() failed\n");
 
 	printk(KERN_INFO KBUILD_MODNAME ": Coming out of suspend..\n");
 

@@ -600,7 +604,8 @@
 
 	video_set_drvdata(radio->videodev, radio);
 	if (video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr)) {
-		warn("Could not register video device");
+		printk(KERN_WARNING KBUILD_MODNAME
+					": Could not register video device\n");
 		video_device_release(radio->videodev);
 		kfree(radio->buffer);
 		kfree(radio);

@@ -618,7 +623,8 @@
 	printk(KERN_INFO KBUILD_MODNAME ": "
 			DRIVER_VERSION " " DRIVER_DESC "\n");
 	if (retval)
-		err("usb_register failed. Error number %d", retval);
+		printk(KERN_ERR KBUILD_MODNAME
+			": usb_register failed. Error number %d\n", retval);
 	return retval;
 }

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

#1 lore raw

I NACK this patch, please reformat to use dev_warn and dev_err rather than using printk. Ideally all drivers should use the dev_ macros when possible and the pr_ macros otherwise. See the device.h and kernel.h for more information.

Regards,

David Ellingsworth

2008/10/16 Alexey Klimov <[email protected]>:

Quoted 7 lines

Hello, all

radio-mr800: remove warn and err messages

Patch removes warn() and err()-statements in radio/radio-mr800.c, because of removing this macros from USB-subsystem.

Quoted 8 lines

--
Best regards, Klimov Alexey

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

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

#2 lore raw
Quoted 11 lines

2008/10/16 Alexey Klimov <[email protected]>:

Quoted 10 lines

Hello, all

radio-mr800: remove warn and err messages

Patch removes warn() and err()-statements in radio/radio-mr800.c, because of removing this macros from USB-subsystem.

--
Best regards, Klimov Alexey

Quoted 8 lines · David Ellingsworth

I NACK this patch, please reformat to use dev_warn and dev_err rather than using printk. Ideally all drivers should use the dev_ macros when possible and the pr_ macros otherwise. See the device.h and kernel.h for more information.

Regards,

David Ellingsworth

David,

Quoting policy on this mailing list is to write your reply below the quoted text -- please stick with this for the future.

Meanwhile, I agree with David, in that the macros should be corrected to the official macros, rather than simply being deleted.

Also, a patch wont go very far without a sign-off. Please see the following for the requirements on a patch for kernel submission:

http://linuxtv.org/hg/v4l-dvb/file/tip/README.patches

Regards,

Mike

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

author
#3 lore raw
In reply to Alexey Klimov

Hello, all
Thanks for input.
This is re-created version of patch. What do you think about this version ?

radio-mr800: remove warn-, err- and info-messages

Patch removes warn(), err() and info() statements in radio/radio-mr800.c, and place dev_warn, dev_info in right places. Printk changed on pr_info and pr_err macro.

Signed-off-by Alexey Klimov <[email protected]>
Signature · 1 line
Best regards, Klimov Alexey
linux/drivers/media/radio/radio-mr800.c +22 -14
diff -r e3b28a9d9635 linux/drivers/media/radio/radio-mr800.c
@@ -362,7 +362,8 @@
 
 	radio->curfreq = f->frequency;
 	if (amradio_setfreq(radio, radio->curfreq) < 0)
-		warn("Set frequency failed");
+		dev_warn(&radio->videodev->dev,
+			"radio-mr800: Set frequency failed\n");
 	return 0;
 }
 

@@ -385,8 +386,7 @@
 
 	for (i = 0; i < ARRAY_SIZE(radio_qctrl); i++) {
 		if (qc->id && qc->id == radio_qctrl[i].id) {
-			memcpy(qc, &(radio_qctrl[i]),
-						sizeof(*qc));
+			memcpy(qc, &(radio_qctrl[i]), sizeof(*qc));
 			return 0;
 		}
 	}

@@ -417,12 +417,14 @@
 	case V4L2_CID_AUDIO_MUTE:
 		if (ctrl->value) {
 			if (amradio_stop(radio) < 0) {
-				warn("amradio_stop() failed");
+				dev_warn(&radio->videodev->dev,
+					"radio-mr800: amradio_stop failed\n");
 				return -1;
 			}
 		} else {
 			if (amradio_start(radio) < 0) {
-				warn("amradio_start() failed");
+				dev_warn(&radio->videodev->dev,
+					"radio-mr800: amradio_start failed\n");
 				return -1;
 			}
 		}

@@ -478,13 +480,15 @@
 	radio->muted = 1;
 
 	if (amradio_start(radio) < 0) {
-		warn("Radio did not start up properly");
+		dev_warn(&radio->videodev->dev,
+			"radio-mr800: Radio did not start up properly\n");
 		radio->users = 0;
 		unlock_kernel();
 		return -EIO;
 	}
 	if (amradio_setfreq(radio, radio->curfreq) < 0)
-		warn("Set frequency failed");
+		dev_warn(&radio->videodev->dev,
+			"radio-mr800: Set frequency failed\n");
 
 	unlock_kernel();
 	return 0;

@@ -511,9 +515,9 @@
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
 	if (amradio_stop(radio) < 0)
-		warn("amradio_stop() failed");
+		dev_warn(&intf->dev, "amradio_stop failed\n");
 
-	info("radio-mr800: Going into suspend..");
+	dev_info(&intf->dev, "Going into suspend..\n");
 
 	return 0;
 }

@@ -524,9 +528,9 @@
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
 	if (amradio_start(radio) < 0)
-		warn("amradio_start() failed");
+		dev_warn(&intf->dev, "amradio_start failed\n");
 
-	info("radio-mr800: Coming out of suspend..");
+	dev_info(&intf->dev, "Coming out of suspend..\n");
 
 	return 0;
 }

@@ -605,7 +609,7 @@
 
 	video_set_drvdata(radio->videodev, radio);
 	if (video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr)) {
-		warn("Could not register video device");
+		dev_warn(&intf->dev, "Could not register video device\n");
 		video_device_release(radio->videodev);
 		kfree(radio->buffer);
 		kfree(radio);

@@ -620,9 +624,13 @@
 {
 	int retval = usb_register(&usb_amradio_driver);
 
-	info(DRIVER_VERSION " " DRIVER_DESC);
+	pr_info(KBUILD_MODNAME
+		": version " DRIVER_VERSION " " DRIVER_DESC "\n");
+
 	if (retval)
-		err("usb_register failed. Error number %d", retval);
+		pr_err(KBUILD_MODNAME
+			": usb_register failed. Error number %d\n", retval);
+
 	return retval;
 }

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

#4 lore raw

2008/10/19 Alexey Klimov <[email protected]>:

Quoted 11 lines

Hello, all
Thanks for input.
This is re-created version of patch. What do you think about this version ?

radio-mr800: remove warn-, err- and info-messages

Patch removes warn(), err() and info() statements in radio/radio-mr800.c, and place dev_warn, dev_info in right places. Printk changed on pr_info and pr_err macro.

Signed-off-by Alexey Klimov <[email protected]>

In the future, please inline the patch as well so it's easier to review and comment on. That said, it looks better than the last. Although, I'm still a bit undecided about this patch as it includes the module/driver name directly in some of the dev_warn calls. I understand why this was done due to how dev_warn behaves, but I think the name should either be defined in a macro or removed entirely. It would be nice to hear what others think about this.

Regards,

David Ellingsworth

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

author
#5 lore raw

Hello, all

Here is new patch, reformatted. Also KBUILD_MODNAME added.

radio-mr800: remove warn-, err- and info-messages

Patch removes warn(), err() and info() statements in radio/radio-mr800.c, and place dev_warn, dev_info in right places.
Printk changed on pr_info and pr_err macro.

Signed-off-by Alexey Klimov <[email protected]>

---

linux/drivers/media/radio/radio-mr800.c +24 -14
diff -r 0b2679b688fe linux/drivers/media/radio/radio-mr800.c
@@ -362,7 +362,8 @@
 
 	radio->curfreq = f->frequency;
 	if (amradio_setfreq(radio, radio->curfreq) < 0)
-		warn("Set frequency failed");
+		dev_warn(&radio->videodev->dev,
+			KBUILD_MODNAME " - set frequency failed\n");
 	return 0;
 }
 

@@ -385,8 +386,7 @@
 
 	for (i = 0; i < ARRAY_SIZE(radio_qctrl); i++) {
 		if (qc->id && qc->id == radio_qctrl[i].id) {
-			memcpy(qc, &(radio_qctrl[i]),
-						sizeof(*qc));
+			memcpy(qc, &(radio_qctrl[i]), sizeof(*qc));
 			return 0;
 		}
 	}

@@ -417,12 +417,16 @@
 	case V4L2_CID_AUDIO_MUTE:
 		if (ctrl->value) {
 			if (amradio_stop(radio) < 0) {
-				warn("amradio_stop() failed");
+				dev_warn(&radio->videodev->dev,
+					KBUILD_MODNAME
+						" - amradio_stop failed\n");
 				return -1;
 			}
 		} else {
 			if (amradio_start(radio) < 0) {
-				warn("amradio_start() failed");
+				dev_warn(&radio->videodev->dev,
+					KBUILD_MODNAME
+						" - amradio_start failed\n");
 				return -1;
 			}
 		}

@@ -478,13 +482,15 @@
 	radio->muted = 1;
 
 	if (amradio_start(radio) < 0) {
-		warn("Radio did not start up properly");
+		dev_warn(&radio->videodev->dev,
+			KBUILD_MODNAME " - radio did not start up properly\n");
 		radio->users = 0;
 		unlock_kernel();
 		return -EIO;
 	}
 	if (amradio_setfreq(radio, radio->curfreq) < 0)
-		warn("Set frequency failed");
+		dev_warn(&radio->videodev->dev,
+			KBUILD_MODNAME " - set frequency failed\n");
 
 	unlock_kernel();
 	return 0;

@@ -511,9 +517,9 @@
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
 	if (amradio_stop(radio) < 0)
-		warn("amradio_stop() failed");
+		dev_warn(&intf->dev, "amradio_stop failed\n");
 
-	info("radio-mr800: Going into suspend..");
+	dev_info(&intf->dev, "going into suspend..\n");
 
 	return 0;
 }

@@ -524,9 +530,9 @@
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
 	if (amradio_start(radio) < 0)
-		warn("amradio_start() failed");
+		dev_warn(&intf->dev, "amradio_start failed\n");
 
-	info("radio-mr800: Coming out of suspend..");
+	dev_info(&intf->dev, "coming out of suspend..\n");
 
 	return 0;
 }

@@ -605,7 +611,7 @@
 
 	video_set_drvdata(radio->videodev, radio);
 	if (video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr)) {
-		warn("Could not register video device");
+		dev_warn(&intf->dev, "could not register video device\n");
 		video_device_release(radio->videodev);
 		kfree(radio->buffer);
 		kfree(radio);

@@ -620,9 +626,13 @@
 {
 	int retval = usb_register(&usb_amradio_driver);
 
-	info(DRIVER_VERSION " " DRIVER_DESC);
+	pr_info(KBUILD_MODNAME
+		": version " DRIVER_VERSION " " DRIVER_DESC "\n");
+
 	if (retval)
-		err("usb_register failed. Error number %d", retval);
+		pr_err(KBUILD_MODNAME
+			": usb_register failed. Error number %d\n", retval);
+
 	return retval;
 }

--

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
#6 lore raw

Hello guys,

Quoted 11 lines · Alexey Klimov

Hello, all

Here is new patch, reformatted. Also KBUILD_MODNAME added.

radio-mr800: remove warn-, err- and info-messages

Patch removes warn(), err() and info() statements in radio/radio-mr800.c, and place dev_warn, dev_info in right places.
Printk changed on pr_info and pr_err macro.

Signed-off-by Alexey Klimov <[email protected]>

Seems sane to my eyes.

Cheers,
Douglas

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

#7 lore raw
In reply to Alexey Klimov
Quoted 4 lines · Alexey Klimov

Hello, all

Here is new patch, reformatted. Also KBUILD_MODNAME added.

Quoted 8 lines

radio-mr800: remove warn-, err- and info-messages

Patch removes warn(), err() and info() statements in radio/radio-mr800.c, and place dev_warn, dev_info in right places.
Printk changed on pr_info and pr_err macro.

Signed-off-by Alexey Klimov <[email protected]>

NACK. KBIULD_MODNAME should not be used except from within module specific function (IE module_init and module_exit routines). It is my understanding that KBUILD_MODNAME expands to the name module when the driver is compiled as a module and the name of the file otherwise. While I understand this is how the previous warn() and err() macros were defined in usb.h I don't believe it's the right thing to do. If you want my ack, you have the following options:

1. Use the same name used in the usb_driver struct
2. Remove the name altogether

If I were writing it, I'd do the following:

#define DRVNAME "radio-mr800"
#define amradio_dev_err(dev, fmt, arg...) dev_err(dev, DRVNAME fmt, ##arg) #define amradio_dev_warn(dev, fmt, arg...) dev_warn(dev, DRVNAME fmt, ##arg)

Update the name member of the usb_driver struct to use DRVNAME rather than the constant string it has there.
Use amradio_dev_err and amradio_dev_warn wherever dev_err or dev_warn is needed.

By doing it this way, the code stays clean, consistent and easily maintainable.

-------------
On a related note, the name stored in the usb_driver struct gets transferred to a device struct by the usb subsystem. In a perfect world, I believe this struct would be the right one to pass to dev_err and dev_warn directly. Unfortunately though, that device struct is buried within usb specific structures and gaining access to is difficult and could be problematic in the future. It is for this reason I don't advocate its use. Having said that, I think it would be OK to use pr_err and pr_warn instead of dev_err and dev_warn here since we really don't have access to the appropriate device struct. That said, I'd also ack a patch which redefined the above macros to use pr_err and pr_warning rather than dev_err and dev_warn and renamed them appropriately.

Regards,

David Ellingsworth

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

#8 lore raw
Quoted 22 lines · David Ellingsworth
Quoted 4 lines · Alexey Klimov

Hello, all

Here is new patch, reformatted. Also KBUILD_MODNAME added.

Quoted 8 lines

radio-mr800: remove warn-, err- and info-messages

Patch removes warn(), err() and info() statements in radio/radio-mr800.c, and place dev_warn, dev_info in right places.
Printk changed on pr_info and pr_err macro.

Signed-off-by Alexey Klimov <[email protected]>

NACK. KBIULD_MODNAME should not be used except from within module specific function (IE module_init and module_exit routines). It is my understanding that KBUILD_MODNAME expands to the name module when the driver is compiled as a module and the name of the file otherwise. While I understand this is how the previous warn() and err() macros were defined in usb.h I don't believe it's the right thing to do. If you want my ack, you have the following options:

Quoted 8 lines

1. Use the same name used in the usb_driver struct
2. Remove the name altogether

If I were writing it, I'd do the following:

#define DRVNAME "radio-mr800"
#define amradio_dev_err(dev, fmt, arg...) dev_err(dev, DRVNAME fmt, ##arg) #define amradio_dev_warn(dev, fmt, arg...) dev_warn(dev, DRVNAME fmt, ##arg)

Minor correction here ^^^^^

It should be:
#define amradio_dev_err(dev, fmt, arg...) dev_err(dev, DRVNAME ": " fmt, ##arg) #define amradio_dev_warn(dev, fmt, arg...) dev_warn(dev, DRVNAME ": " fmt, ##arg)

- David Ellingsworth

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

author
#9 lore raw

Hello, all

Quoted 10 lines · David Ellingsworth
Quoted 10 lines

you want my ack, you have the following options:

1. Use the same name used in the usb_driver struct
2. Remove the name altogether

If I were writing it, I'd do the following:

#define DRVNAME "radio-mr800"
#define amradio_dev_err(dev, fmt, arg...) dev_err(dev, DRVNAME fmt, ##arg) #define amradio_dev_warn(dev, fmt, arg...) dev_warn(dev, DRVNAME fmt, ##arg)

Quoted 8 lines

Minor correction here ^^^^^

It should be:
#define amradio_dev_err(dev, fmt, arg...) dev_err(dev, DRVNAME ": " fmt, ##arg) #define amradio_dev_warn(dev, fmt, arg...) dev_warn(dev, DRVNAME ": " fmt, ##arg)

- David Ellingsworth

You idea is great. I made patch, but have few questions.

May i use " - " in amradio_dev_info for example, instead of using ..DRVNAME ": " fmt.. in defined macros? There are a lot of ":" in messages if using ":".

May i use amradio_dev_info for cases where i want &radio->videodev->dev ? And may i use usual dev_warn where intf->dev is more apropriate ? Or you want me to use amradio_dev_warn macroses everywhere ?

David, what do you think ?

Best regards,
Alexey Klimov

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

author
#10 lore raw
In reply to David Ellingsworth

Hello, again
What do you think about this ?

radio-mr800: remove warn-, err- and info-messages

Patch removes warn(), err() and info() statements in media/radio/radio-mr800.c, and place dev_warn, dev_info in right places. Printk changed on pr_info and pr_err macro.

Signed-off-by Alexey Klimov <[email protected]>

---

linux/drivers/media/radio/radio-mr800.c +26 -14
diff -r db5374be1876 linux/drivers/media/radio/radio-mr800.c
@@ -72,6 +72,10 @@
 
 #define USB_AMRADIO_VENDOR 0x07ca
 #define USB_AMRADIO_PRODUCT 0xb800
+
+/* dev_warn macro with driver name */
+#define DRIVERNAME "radio-mr800"
+#define amradio_dev_warn(dev, fmt, arg...) dev_warn(dev, DRIVERNAME fmt, ##arg)
 
 /* Probably USB_TIMEOUT should be modified in module parameter */
 #define BUFFER_LENGTH 8

@@ -362,7 +366,8 @@
 
 	radio->curfreq = f->frequency;
 	if (amradio_setfreq(radio, radio->curfreq) < 0)
-		warn("Set frequency failed");
+		amradio_dev_warn(&radio->videodev->dev,
+			" - set frequency failed\n");
 	return 0;
 }
 

@@ -385,8 +390,7 @@
 
 	for (i = 0; i < ARRAY_SIZE(radio_qctrl); i++) {
 		if (qc->id && qc->id == radio_qctrl[i].id) {
-			memcpy(qc, &(radio_qctrl[i]),
-						sizeof(*qc));
+			memcpy(qc, &(radio_qctrl[i]), sizeof(*qc));
 			return 0;
 		}
 	}

@@ -417,12 +421,14 @@
 	case V4L2_CID_AUDIO_MUTE:
 		if (ctrl->value) {
 			if (amradio_stop(radio) < 0) {
-				warn("amradio_stop() failed");
+				amradio_dev_warn(&radio->videodev->dev,
+					" - amradio_stop failed\n");
 				return -1;
 			}
 		} else {
 			if (amradio_start(radio) < 0) {
-				warn("amradio_start() failed");
+				amradio_dev_warn(&radio->videodev->dev,
+					" - amradio_start failed\n");
 				return -1;
 			}
 		}

@@ -478,13 +484,15 @@
 	radio->muted = 1;
 
 	if (amradio_start(radio) < 0) {
-		warn("Radio did not start up properly");
+		amradio_dev_warn(&radio->videodev->dev,
+			" - radio did not start up properly\n");
 		radio->users = 0;
 		unlock_kernel();
 		return -EIO;
 	}
 	if (amradio_setfreq(radio, radio->curfreq) < 0)
-		warn("Set frequency failed");
+		amradio_dev_warn(&radio->videodev->dev,
+			" - set frequency failed\n");
 
 	unlock_kernel();
 	return 0;

@@ -511,9 +519,9 @@
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
 	if (amradio_stop(radio) < 0)
-		warn("amradio_stop() failed");
+		dev_warn(&intf->dev, "amradio_stop failed\n");
 
-	info("radio-mr800: Going into suspend..");
+	dev_info(&intf->dev, "going into suspend..\n");
 
 	return 0;
 }

@@ -524,9 +532,9 @@
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
 	if (amradio_start(radio) < 0)
-		warn("amradio_start() failed");
+		dev_warn(&intf->dev, "amradio_start failed\n");
 
-	info("radio-mr800: Coming out of suspend..");
+	dev_info(&intf->dev, "coming out of suspend..\n");
 
 	return 0;
 }

@@ -605,7 +613,7 @@
 
 	video_set_drvdata(radio->videodev, radio);
 	if (video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr)) {
-		warn("Could not register video device");
+		dev_warn(&intf->dev, "could not register video device\n");
 		video_device_release(radio->videodev);
 		kfree(radio->buffer);
 		kfree(radio);

@@ -620,9 +628,13 @@
 {
 	int retval = usb_register(&usb_amradio_driver);
 
-	info(DRIVER_VERSION " " DRIVER_DESC);
+	pr_info(KBUILD_MODNAME
+		": version " DRIVER_VERSION " " DRIVER_DESC "\n");
+
 	if (retval)
-		err("usb_register failed. Error number %d", retval);
+		pr_err(KBUILD_MODNAME
+			": usb_register failed. Error number %d\n", retval);
+
 	return retval;
 }
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
#11 lore raw

[snip]

linux/drivers/media/radio/radio-mr800.c line 77

Quoted 5 lines

#define USB_AMRADIO_VENDOR 0x07ca
#define USB_AMRADIO_PRODUCT 0xb800

linux/drivers/media/radio/radio-mr800.c +3 -0
+
+/* dev_warn macro with driver name */
+#define DRIVERNAME "radio-mr800"

should probably be DRIVER_NAME

Quoted 1 line

+#define amradio_dev_warn(dev, fmt, arg...) dev_warn(dev, DRIVERNAME fmt, ##arg)

Add " - " before fmt and remove it from all current uses of amradio_dev_warn

Review

Quoted 4 lines

/* Probably USB_TIMEOUT should be modified in module parameter */ #define BUFFER_LENGTH 8

(quoted diff) +0 -0
@@ -362,7 +366,8 @@

[snip]

Please update the name member of the usb_driver struct to use the DRIVER_NAME macro as well. Everything else looks good.

Regards,

David Ellingsworth

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

#13 lore raw
Quoted 19 lines · Carl Karsten
Quoted 9 lines · David Ellingsworth

[snip]

linux/drivers/media/radio/radio-mr800.c line 77

Quoted 5 lines

#define USB_AMRADIO_VENDOR 0x07ca
#define USB_AMRADIO_PRODUCT 0xb800

linux/drivers/media/radio/radio-mr800.c +3 -0
+
+/* dev_warn macro with driver name */
+#define DRIVERNAME "radio-mr800"

should probably be DRIVER_NAME

I think it should be unique, based on

vivi: rename MODULE_NAME macro to VIVI_MODULE_NAME to avoid namespace conflicts
From: Mauro Carvalho Chehab <[email protected]>

(quoted diff) +1 -1
-#define MODULE_NAME "vivi"
+#define VIVI_MODULE_NAME "vivi"
Quoted 9 lines

http://linuxtv.org/hg/v4l-dvb/rev/58b95134acb8.

and
#define VIVI_MODULE_NAME "vivi"

http://linuxtv.org/hg/v4l-dvb/file/50e4cdc46320/linux/drivers/media/video/vivi.c

Carl K

Carl is right. Carl, please stay on-list next time.

Regards,

David Ellingsworth

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

author
#14 lore raw
In reply to David Ellingsworth

Patch removes warn(), err() and info() statements in

media/radio/radio-mr800.c, and place dev_warn, dev_info in right places. Printk changed on pr_info and pr_err macro.

Signed-off-by Alexey Klimov <[email protected]>

--

linux/drivers/media/radio/radio-mr800.c +26 -14
diff -r db5374be1876 linux/drivers/media/radio/radio-mr800.c
@@ -72,6 +72,10 @@
 
 #define USB_AMRADIO_VENDOR 0x07ca
 #define USB_AMRADIO_PRODUCT 0xb800
+
+/* dev_warn macro with driver name */
+#define MR800_DRIVER_NAME "radio-mr800"
+#define amradio_dev_warn(dev, fmt, arg...) dev_warn(dev, MR800_DRIVER_NAME " - " fmt, ##arg)
 
 /* Probably USB_TIMEOUT should be modified in module parameter */
 #define BUFFER_LENGTH 8

@@ -362,7 +366,8 @@
 
 	radio->curfreq = f->frequency;
 	if (amradio_setfreq(radio, radio->curfreq) < 0)
-		warn("Set frequency failed");
+		amradio_dev_warn(&radio->videodev->dev,
+			"set frequency failed\n");
 	return 0;
 }
 

@@ -385,8 +390,7 @@
 
 	for (i = 0; i < ARRAY_SIZE(radio_qctrl); i++) {
 		if (qc->id && qc->id == radio_qctrl[i].id) {
-			memcpy(qc, &(radio_qctrl[i]),
-						sizeof(*qc));
+			memcpy(qc, &(radio_qctrl[i]), sizeof(*qc));
 			return 0;
 		}
 	}

@@ -417,12 +421,14 @@
 	case V4L2_CID_AUDIO_MUTE:
 		if (ctrl->value) {
 			if (amradio_stop(radio) < 0) {
-				warn("amradio_stop() failed");
+				amradio_dev_warn(&radio->videodev->dev,
+					"amradio_stop failed\n");
 				return -1;
 			}
 		} else {
 			if (amradio_start(radio) < 0) {
-				warn("amradio_start() failed");
+				amradio_dev_warn(&radio->videodev->dev,
+					"amradio_start failed\n");
 				return -1;
 			}
 		}

@@ -478,13 +484,15 @@
 	radio->muted = 1;
 
 	if (amradio_start(radio) < 0) {
-		warn("Radio did not start up properly");
+		amradio_dev_warn(&radio->videodev->dev,
+			"radio did not start up properly\n");
 		radio->users = 0;
 		unlock_kernel();
 		return -EIO;
 	}
 	if (amradio_setfreq(radio, radio->curfreq) < 0)
-		warn("Set frequency failed");
+		amradio_dev_warn(&radio->videodev->dev,
+			"set frequency failed\n");
 
 	unlock_kernel();
 	return 0;

@@ -511,9 +519,9 @@
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
 	if (amradio_stop(radio) < 0)
-		warn("amradio_stop() failed");
+		dev_warn(&intf->dev, "amradio_stop failed\n");
 
-	info("radio-mr800: Going into suspend..");
+	dev_info(&intf->dev, "going into suspend..\n");
 
 	return 0;
 }

@@ -524,9 +532,9 @@
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
 	if (amradio_start(radio) < 0)
-		warn("amradio_start() failed");
+		dev_warn(&intf->dev, "amradio_start failed\n");
 
-	info("radio-mr800: Coming out of suspend..");
+	dev_info(&intf->dev, "coming out of suspend..\n");
 
 	return 0;
 }

@@ -605,7 +613,7 @@
 
 	video_set_drvdata(radio->videodev, radio);
 	if (video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr)) {
-		warn("Could not register video device");
+		dev_warn(&intf->dev, "could not register video device\n");
 		video_device_release(radio->videodev);
 		kfree(radio->buffer);
 		kfree(radio);

@@ -620,9 +628,13 @@
 {
 	int retval = usb_register(&usb_amradio_driver);
 
-	info(DRIVER_VERSION " " DRIVER_DESC);
+	pr_info(KBUILD_MODNAME
+		": version " DRIVER_VERSION " " DRIVER_DESC "\n");
+
 	if (retval)
-		err("usb_register failed. Error number %d", retval);
+		pr_err(KBUILD_MODNAME
+			": usb_register failed. Error number %d\n", retval);
+
 	return retval;
 }
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
author
#16 lore raw

And this version ? :)
Maybe it's good idea to remove KBUILD_MODNAME in pr_info and place MR800_DRIVER_NAME there ?

Patch removes warn(), err() and info() statements in media/radio/radio-mr800.c, and place dev_warn, dev_info in right places. Printk changed on pr_info and pr_err macro. Also new macro amradio_dev_warn defined.

Signed-off-by Alexey Klimov <[email protected]>

--

linux/drivers/media/radio/radio-mr800.c +27 -15
diff -r db5374be1876 linux/drivers/media/radio/radio-mr800.c
@@ -72,6 +72,10 @@
 
 #define USB_AMRADIO_VENDOR 0x07ca
 #define USB_AMRADIO_PRODUCT 0xb800
+
+/* dev_warn macro with driver name */
+#define MR800_DRIVER_NAME "radio-mr800"
+#define amradio_dev_warn(dev, fmt, arg...) dev_warn(dev, MR800_DRIVER_NAME " - " fmt, ##arg)
 
 /* Probably USB_TIMEOUT should be modified in module parameter */
 #define BUFFER_LENGTH 8

@@ -155,7 +159,7 @@
 
 /* USB subsystem interface */
 static struct usb_driver usb_amradio_driver = {
-	.name			= "radio-mr800",
+	.name			= MR800_DRIVER_NAME,
 	.probe			= usb_amradio_probe,
 	.disconnect		= usb_amradio_disconnect,
 	.suspend		= usb_amradio_suspend,

@@ -362,7 +366,8 @@
 
 	radio->curfreq = f->frequency;
 	if (amradio_setfreq(radio, radio->curfreq) < 0)
-		warn("Set frequency failed");
+		amradio_dev_warn(&radio->videodev->dev,
+			"set frequency failed\n");
 	return 0;
 }
 

@@ -385,8 +390,7 @@
 
 	for (i = 0; i < ARRAY_SIZE(radio_qctrl); i++) {
 		if (qc->id && qc->id == radio_qctrl[i].id) {
-			memcpy(qc, &(radio_qctrl[i]),
-						sizeof(*qc));
+			memcpy(qc, &(radio_qctrl[i]), sizeof(*qc));
 			return 0;
 		}
 	}

@@ -417,12 +421,14 @@
 	case V4L2_CID_AUDIO_MUTE:
 		if (ctrl->value) {
 			if (amradio_stop(radio) < 0) {
-				warn("amradio_stop() failed");
+				amradio_dev_warn(&radio->videodev->dev,
+					"amradio_stop failed\n");
 				return -1;
 			}
 		} else {
 			if (amradio_start(radio) < 0) {
-				warn("amradio_start() failed");
+				amradio_dev_warn(&radio->videodev->dev,
+					"amradio_start failed\n");
 				return -1;
 			}
 		}

@@ -478,13 +484,15 @@
 	radio->muted = 1;
 
 	if (amradio_start(radio) < 0) {
-		warn("Radio did not start up properly");
+		amradio_dev_warn(&radio->videodev->dev,
+			"radio did not start up properly\n");
 		radio->users = 0;
 		unlock_kernel();
 		return -EIO;
 	}
 	if (amradio_setfreq(radio, radio->curfreq) < 0)
-		warn("Set frequency failed");
+		amradio_dev_warn(&radio->videodev->dev,
+			"set frequency failed\n");
 
 	unlock_kernel();
 	return 0;

@@ -511,9 +519,9 @@
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
 	if (amradio_stop(radio) < 0)
-		warn("amradio_stop() failed");
+		dev_warn(&intf->dev, "amradio_stop failed\n");
 
-	info("radio-mr800: Going into suspend..");
+	dev_info(&intf->dev, "going into suspend..\n");
 
 	return 0;
 }

@@ -524,9 +532,9 @@
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
 	if (amradio_start(radio) < 0)
-		warn("amradio_start() failed");
+		dev_warn(&intf->dev, "amradio_start failed\n");
 
-	info("radio-mr800: Coming out of suspend..");
+	dev_info(&intf->dev, "coming out of suspend..\n");
 
 	return 0;
 }

@@ -605,7 +613,7 @@
 
 	video_set_drvdata(radio->videodev, radio);
 	if (video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr)) {
-		warn("Could not register video device");
+		dev_warn(&intf->dev, "could not register video device\n");
 		video_device_release(radio->videodev);
 		kfree(radio->buffer);
 		kfree(radio);

@@ -620,9 +628,13 @@
 {
 	int retval = usb_register(&usb_amradio_driver);
 
-	info(DRIVER_VERSION " " DRIVER_DESC);
+	pr_info(KBUILD_MODNAME
+		": version " DRIVER_VERSION " " DRIVER_DESC "\n");
+
 	if (retval)
-		err("usb_register failed. Error number %d", retval);
+		pr_err(KBUILD_MODNAME
+			": usb_register failed. Error number %d\n", retval);
+
 	return retval;
 }
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
#17 lore raw
Quoted 4 lines · Alexey Klimov

And this version ? :)
Maybe it's good idea to remove KBUILD_MODNAME in pr_info and place MR800_DRIVER_NAME there ?

It's your choice. I'm ok with the use of KBUILD_MODNAME there as it's within a module_init function. If you use the MR800_DRIVER_NAME macro, I'd prefer to see another macro definition similar to the other one, something like amradio_err which expands to calling pr_err(MR800_DRIVER_NAME... for consistency. Let me know if you want me to ack this one, otherwise send a revised patch.

Regards,

David Ellingsworth

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

author
#18 lore raw

PATCH 1/1 radio-mr800: remove warn, info and err messages

Patch removes warn(), err() and info() statements in drivers/media/radio/radio-mr800.c, and place dev_warn, dev_info in right places.
Printk changed on pr_info and pr_err macro. Also new macro amradio_dev_warn defined. Name in usb driver struct changed on MR800_DRIVER_NAME.

Signed-off-by Alexey Klimov <[email protected]>

--

linux/drivers/media/radio/radio-mr800.c +28 -15
diff -r db5374be1876 linux/drivers/media/radio/radio-mr800.c
@@ -72,6 +72,11 @@
 
 #define USB_AMRADIO_VENDOR 0x07ca
 #define USB_AMRADIO_PRODUCT 0xb800
+
+/* dev_warn macro with driver name */
+#define MR800_DRIVER_NAME "radio-mr800"
+#define amradio_dev_warn(dev, fmt, arg...)				\
+		dev_warn(dev, MR800_DRIVER_NAME " - " fmt, ##arg)
 
 /* Probably USB_TIMEOUT should be modified in module parameter */
 #define BUFFER_LENGTH 8

@@ -155,7 +160,7 @@
 
 /* USB subsystem interface */
 static struct usb_driver usb_amradio_driver = {
-	.name			= "radio-mr800",
+	.name			= MR800_DRIVER_NAME,
 	.probe			= usb_amradio_probe,
 	.disconnect		= usb_amradio_disconnect,
 	.suspend		= usb_amradio_suspend,

@@ -362,7 +367,8 @@
 
 	radio->curfreq = f->frequency;
 	if (amradio_setfreq(radio, radio->curfreq) < 0)
-		warn("Set frequency failed");
+		amradio_dev_warn(&radio->videodev->dev,
+			"set frequency failed\n");
 	return 0;
 }
 

@@ -385,8 +391,7 @@
 
 	for (i = 0; i < ARRAY_SIZE(radio_qctrl); i++) {
 		if (qc->id && qc->id == radio_qctrl[i].id) {
-			memcpy(qc, &(radio_qctrl[i]),
-						sizeof(*qc));
+			memcpy(qc, &(radio_qctrl[i]), sizeof(*qc));
 			return 0;
 		}
 	}

@@ -417,12 +422,14 @@
 	case V4L2_CID_AUDIO_MUTE:
 		if (ctrl->value) {
 			if (amradio_stop(radio) < 0) {
-				warn("amradio_stop() failed");
+				amradio_dev_warn(&radio->videodev->dev,
+					"amradio_stop failed\n");
 				return -1;
 			}
 		} else {
 			if (amradio_start(radio) < 0) {
-				warn("amradio_start() failed");
+				amradio_dev_warn(&radio->videodev->dev,
+					"amradio_start failed\n");
 				return -1;
 			}
 		}

@@ -478,13 +485,15 @@
 	radio->muted = 1;
 
 	if (amradio_start(radio) < 0) {
-		warn("Radio did not start up properly");
+		amradio_dev_warn(&radio->videodev->dev,
+			"radio did not start up properly\n");
 		radio->users = 0;
 		unlock_kernel();
 		return -EIO;
 	}
 	if (amradio_setfreq(radio, radio->curfreq) < 0)
-		warn("Set frequency failed");
+		amradio_dev_warn(&radio->videodev->dev,
+			"set frequency failed\n");
 
 	unlock_kernel();
 	return 0;

@@ -511,9 +520,9 @@
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
 	if (amradio_stop(radio) < 0)
-		warn("amradio_stop() failed");
+		dev_warn(&intf->dev, "amradio_stop failed\n");
 
-	info("radio-mr800: Going into suspend..");
+	dev_info(&intf->dev, "going into suspend..\n");
 
 	return 0;
 }

@@ -524,9 +533,9 @@
 	struct amradio_device *radio = usb_get_intfdata(intf);
 
 	if (amradio_start(radio) < 0)
-		warn("amradio_start() failed");
+		dev_warn(&intf->dev, "amradio_start failed\n");
 
-	info("radio-mr800: Coming out of suspend..");
+	dev_info(&intf->dev, "coming out of suspend..\n");
 
 	return 0;
 }

@@ -605,7 +614,7 @@
 
 	video_set_drvdata(radio->videodev, radio);
 	if (video_register_device(radio->videodev, VFL_TYPE_RADIO, radio_nr)) {
-		warn("Could not register video device");
+		dev_warn(&intf->dev, "could not register video device\n");
 		video_device_release(radio->videodev);
 		kfree(radio->buffer);
 		kfree(radio);

@@ -620,9 +629,13 @@
 {
 	int retval = usb_register(&usb_amradio_driver);
 
-	info(DRIVER_VERSION " " DRIVER_DESC);
+	pr_info(KBUILD_MODNAME
+		": version " DRIVER_VERSION " " DRIVER_DESC "\n");
+
 	if (retval)
-		err("usb_register failed. Error number %d", retval);
+		pr_err(KBUILD_MODNAME
+			": usb_register failed. Error number %d\n", retval);
+
 	return retval;
 }
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