Re: [PATCH 2/9] USB: SisUSB2VGA: Convert printk to dev_* macros

Hi Felipe,

on 2007-08-10 15:34 Felipe Balbi wrote:
> -					printk(KERN_ERR
> -						"sisusbvga[%d]: Wrote %zd of "
> -						"%d bytes, error %d\n",
> -						sisusb->minor, *bytes_written,
> -						length, ret);
> +					dev_err(&sisusb->sisusb_dev->dev, "Wrote %zd of %d bytes, error %d\n",
> +						*bytes_written, length, ret);

It has been said on LKML recently that the 80 column limit isn't
absolute and can be exceeded if the readability of the code would
otherwise suffer, but I don't think this is such a case. Why don't
you put the string literal on a line by its own, like so?

					dev_err(&sisusb->sisusb_dev->dev,
						"Wrote %zd of %d bytes, error %d\n",
						*bytes_written, length, ret);

(This applies in many places throughout your patch.)

> @@ -1446,10 +1440,10 @@ sisusb_testreadwrite(struct sisusb_usb_data *sisusb)
>      sisusb_copy_memory(sisusb, srcbuffer, sisusb->vrambase, 7, &dummy);
>  
>      for(i = 1; i <= 7; i++) {
> -        printk(KERN_DEBUG "sisusb: rwtest %d bytes\n", i);
> +        dev_dbg(sisusb->sisusb_dev->dev, "sisusb: rwtest %d bytes\n", i);

Remove the "sisusb:" label here like in the other cases.

> @@ -3202,8 +3176,7 @@ static int sisusb_probe(struct usb_interface *intf,
>  
>  	/* Register device */
>  	if ((retval = usb_register_dev(intf, &usb_sisusb_class))) {
> -		printk(KERN_ERR
> -			"sisusb: Failed to get a minor for device %d\n",
> +		dev_err(&sisusb->sisusb_dev->dev, "Failed to get a minor for device %d\n",
>  			dev->devnum);

sisusb->sisusb_dev is not yet set at this point, but the device
pointer is available in the local variable dev. Also, is printing
dev->devnum really helpful here, when dev_err has already inserted
dev->dev->bus_id at the beginning? So make this simply:

		dev_err(&dev->dev, "Failed to get a minor\n"):

> @@ -3221,7 +3194,7 @@ static int sisusb_probe(struct usb_interface *intf,
>  	sisusb->ibufsize = SISUSB_IBUF_SIZE;
>  	if (!(sisusb->ibuf = usb_buffer_alloc(dev, SISUSB_IBUF_SIZE,
>  					GFP_KERNEL, &sisusb->transfer_dma_in))) {
> -		printk(memfail, "input", sisusb->minor);
> +		dev_err(&sisusb->sisusb_dev->dev, "Failed to allocate memory for input buffer");

Lacks a final newline character. Also, though sisusb->sisusb_dev is
valid now,

		dev_err(&dev->dev, "Failed to allocate memory for input buffer\n");

is still more efficient. (Holds for the rest of the function, too.)

> @@ -3424,6 +3375,7 @@ static struct usb_driver sisusb_driver = {
>  static int __init usb_sisusb_init(void)
>  {
>  	int retval;
> +	struct sisusb_usb_data *sisusb;
>  
>  #ifdef INCL_SISUSB_CON
>  	sisusb_init_concode();
> @@ -3431,10 +3383,9 @@ static int __init usb_sisusb_init(void)
>  
>  	if (!(retval = usb_register(&sisusb_driver))) {
>  
> -		printk(KERN_INFO "sisusb: Driver version %d.%d.%d\n",
> -			SISUSB_VERSION, SISUSB_REVISION, SISUSB_PATCHLEVEL);
> -		printk(KERN_INFO
> -			"sisusb: Copyright (C) 2005 Thomas Winischhofer\n");
> +		dev_info(&sisusb->sisusb_dev->dev, "Driver version %d.%d.%d\n", SISUSB_VERSION,
> +				SISUSB_REVISION, SISUSB_PATCHLEVEL);
> +		dev_info(&sisusb->sisusb_dev->dev, "sisusb: Copyright (C) 2005 Thomas Winischhofer\n");

That won't work. The sisusb pointer is never set. In fact, this is the
very kind of situation where you cannot use the dev_ macros at all,
because no device structure is available. You could use the macros
from usb.h which prefix the source file name instead:

		info("Driver version %d.%d.%d",
			SISUSB_VERSION, SISUSB_REVISION, SISUSB_PATCHLEVEL);
		info("Copyright (C) 2005 Thomas Winischhofer");

(note: no newline characters - these macros add one by their own), but
the result isn't so pretty:

drivers/usb/misc/sisusbvga/sisusb.c: Driver version x.y.z
drivers/usb/misc/sisusbvga/sisusb.c: Copyright (C) 2005 Thomas Winischhofer

So I'd say leave these alone.

HTH

-- 
Tilman Schmidt                          E-Mail: [email protected]
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3rc1 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGxwuhMdB4Whm86/kRAtW0AJ44gDhV5/TrUlwx8rMzdLOJybQl0QCfdOjl
/4WtZwj4rdH67ALSV0f3MvA=
=ikM4
-----END PGP SIGNATURE-----