[patch] radio-mr800: remove warn- and err- messages

[patch] radio-mr800: remove warn- and err- messages

[Alexey Klimov]

[-- Attachment #1: Type: text/plain, Size: 206 bytes --]

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

[-- Attachment #2: radio-mr800-remove-warn-and-err.patch --]
[-- Type: application/octet-stream, Size: 2491 bytes --]

diff -r 270723a73207 linux/drivers/media/radio/radio-mr800.c
--- a/linux/drivers/media/radio/radio-mr800.c	Tue Oct 14 20:46:52 2008 +0400
+++ b/linux/drivers/media/radio/radio-mr800.c	Tue Oct 14 21:22:15 2008 +0400
@@ -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;
 }
 

[-- Attachment #3: Type: text/plain, Size: 164 bytes --]

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

Re: [patch] radio-mr800: remove warn- and err- messages

[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

2008/10/16 Alexey Klimov <klimov.linux@gmail.com>:
> 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
>
> --
> video4linux-list mailing list
> Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list
>

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

Re: [patch] radio-mr800: remove warn- and err- messages

[Michael Krufky]

> 2008/10/16 Alexey Klimov <klimov.linux@gmail.com>:
>> 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
>>

On Thu, Oct 16, 2008 at 3:17 PM, David Ellingsworth
<david@identd.dyndns.org> wrote:
> 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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [patch] radio-mr800: remove warn- and err- messages

[Alexey Klimov]

[-- Attachment #1: Type: text/plain, Size: 409 bytes --]

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 <klimov.linux@gmail.com>

-- 
Best regards, Klimov Alexey

[-- Attachment #2: radio-mr800-remove-warn-info-err.patch --]
[-- Type: application/octet-stream, Size: 2934 bytes --]

diff -r e3b28a9d9635 linux/drivers/media/radio/radio-mr800.c
--- a/linux/drivers/media/radio/radio-mr800.c	Sat Oct 18 18:15:54 2008 +0400
+++ b/linux/drivers/media/radio/radio-mr800.c	Sat Oct 18 18:55:28 2008 +0400
@@ -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;
 }
 

[-- Attachment #3: Type: text/plain, Size: 164 bytes --]

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

Re: [patch] radio-mr800: remove warn- and err- messages

[David Ellingsworth]

2008/10/19 Alexey Klimov <klimov.linux@gmail.com>:
> 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 <klimov.linux@gmail.com>

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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [patch] radio-mr800: remove warn- and err- messages

[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 <klimov.linux@gmail.com>

---

diff -r 0b2679b688fe linux/drivers/media/radio/radio-mr800.c
--- a/linux/drivers/media/radio/radio-mr800.c	Tue Oct 28 14:15:21 2008 +0300
+++ b/linux/drivers/media/radio/radio-mr800.c	Tue Oct 28 20:49:54 2008 +0300
@@ -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;
 }
 
--

-- 
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

Re: [patch] radio-mr800: remove warn- and err- messages

[Douglas Schilling Landgraf]

Hello guys,

On Tue, Oct 28, 2008 at 4:05 PM, Alexey Klimov <klimov.linux@gmail.com> wrote:
> 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 <klimov.linux@gmail.com>

Seems sane to my eyes.

Cheers,
Douglas

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

Re: [patch] radio-mr800: remove warn- and err- messages

[David Ellingsworth]

On Tue, Oct 28, 2008 at 1:05 PM, Alexey Klimov <klimov.linux@gmail.com> wrote:
> 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 <klimov.linux@gmail.com>
>

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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [patch] radio-mr800: remove warn- and err- messages

[David Ellingsworth]

On Wed, Oct 29, 2008 at 1:08 PM, David Ellingsworth
<david@identd.dyndns.org> wrote:
> On Tue, Oct 28, 2008 at 1:05 PM, Alexey Klimov <klimov.linux@gmail.com> wrote:
>> 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 <klimov.linux@gmail.com>
>>
>
> 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)
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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [patch] radio-mr800: remove warn- and err- messages

[Alexey Klimov]

Hello, all

On Wed, 2008-10-29 at 13:12 -0400, David Ellingsworth wrote:
> > 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)
> 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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [patch] radio-mr800: remove warn- and err- messages

[Alexey Klimov]

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 <klimov.linux@gmail.com>

---
diff -r db5374be1876 linux/drivers/media/radio/radio-mr800.c
--- a/linux/drivers/media/radio/radio-mr800.c	Mon Nov 03 04:26:47 2008 +0300
+++ b/linux/drivers/media/radio/radio-mr800.c	Mon Nov 03 18:52:11 2008 +0300
@@ -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;
 }
 


-- 
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

Re: [patch] radio-mr800: remove warn- and err- messages

[David Ellingsworth]

[snip]
>  #define USB_AMRADIO_VENDOR 0x07ca
>  #define USB_AMRADIO_PRODUCT 0xb800
> +
> +/* dev_warn macro with driver name */
> +#define DRIVERNAME "radio-mr800"

should probably be DRIVER_NAME

> +#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

>
>  /* Probably USB_TIMEOUT should be modified in module parameter */
>  #define BUFFER_LENGTH 8
> @@ -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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [patch] radio-mr800: remove warn- and err- messages

[David Ellingsworth]

On Mon, Nov 3, 2008 at 11:46 AM, Carl Karsten <carl@personnelware.com> wrote:
> David Ellingsworth wrote:
>> [snip]
>>>  #define USB_AMRADIO_VENDOR 0x07ca
>>>  #define USB_AMRADIO_PRODUCT 0xb800
>>> +
>>> +/* 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 <mchehab@infradead.org>
>
> -#define MODULE_NAME "vivi"
> +#define VIVI_MODULE_NAME "vivi"
>
> 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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [patch] radio-mr800: remove warn- and err- messages

[Alexey Klimov]

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 <klimov.linux@gmail.com>

--
diff -r db5374be1876 linux/drivers/media/radio/radio-mr800.c
--- a/linux/drivers/media/radio/radio-mr800.c	Mon Nov 03 04:26:47 2008 +0300
+++ b/linux/drivers/media/radio/radio-mr800.c	Mon Nov 03 20:20:24 2008 +0300
@@ -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;
 }
 



-- 
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

Re: [patch] radio-mr800: remove warn- and err- messages

[David Ellingsworth]

Looks good, but still missing one small detail. Set the ".name"
attribute/member of the usb_driver struct to the MR800_DRIVER_NAME
macro. This makes renaming the driver and updating the messages a
simple modification of the MR800_DRIVER_NAME macro.

Regards,

David Ellingsworth

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

Re: [patch] radio-mr800: remove warn- and err- messages

[Alexey Klimov]

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 <klimov.linux@gmail.com>

--

diff -r db5374be1876 linux/drivers/media/radio/radio-mr800.c
--- a/linux/drivers/media/radio/radio-mr800.c	Mon Nov 03 04:26:47 2008 +0300
+++ b/linux/drivers/media/radio/radio-mr800.c	Mon Nov 03 20:46:54 2008 +0300
@@ -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;
 }
 


-- 
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

Re: [patch] radio-mr800: remove warn- and err- messages

[David Ellingsworth]

On Mon, Nov 3, 2008 at 12:51 PM, Alexey Klimov <klimov.linux@gmail.com> wrote:
> 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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

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

[Alexey Klimov]

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 <klimov.linux@gmail.com>

--

diff -r db5374be1876 linux/drivers/media/radio/radio-mr800.c
--- a/linux/drivers/media/radio/radio-mr800.c	Mon Nov 03 04:26:47 2008 +0300
+++ b/linux/drivers/media/radio/radio-mr800.c	Tue Nov 04 17:55:24 2008 +0300
@@ -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;
 }
 


-- 
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

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

[David Ellingsworth]

Mauro,

You may add my acked by on this one.

Acked-by David Ellingsworth <david@identd.dyndns.org>

Regards,

David Ellingsworth

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