[PATCH 1/3] xgifb: checkpatch cleanup, braces

[PATCH 1/3] xgifb: checkpatch cleanup, braces

[Sam Hansen]

Cleaned up XGI_main_26.c and removed some unneeded braces to keep with code
conventions.

Signed-off-by: Sam Hansen <solid.se7en@gmail.com>
---
 drivers/staging/xgifb/XGI_main_26.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
index 2502c49..dd97b73 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -2033,13 +2033,12 @@ static int __devinit xgifb_probe(struct pci_dev *pdev,
 		xgifb_info->hasVB = HASVB_NONE;
 	} else if (xgifb_info->chip == XG21) {
 		CR38 = xgifb_reg_get(XGICR, 0x38);
-		if ((CR38&0xE0) == 0xC0) {
+		if ((CR38&0xE0) == 0xC0)
 			xgifb_info->display2 = XGIFB_DISP_LCD;
-		} else if ((CR38&0xE0) == 0x60) {
+		else if ((CR38&0xE0) == 0x60)
 			xgifb_info->hasVB = HASVB_CHRONTEL;
-		} else {
+		else
 			xgifb_info->hasVB = HASVB_NONE;
-		}
 	} else {
 		XGIfb_get_VB_type(xgifb_info);
 	}
@@ -2147,9 +2146,6 @@ static int __devinit xgifb_probe(struct pci_dev *pdev,
 					if (tmp & 0x20) {
 						tmp = xgifb_reg_get(
 							XGIPART1, 0x13);
-						if (tmp & 0x04) {
-							/* XGI_Pr.XGI_UseLCDA = 1; */
-						}
 					}
 				}
 			}
-- 
1.7.5.4

[PATCH 2/3] xgifb: checkpatch cleanup __func__

[Sam Hansen]

Replaced an instance of __FUNCTION__ with __func__ in XGI_main_26.c.

Signed-off-by: Sam Hansen <solid.se7en@gmail.com>
---
 drivers/staging/xgifb/XGI_main_26.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
index dd97b73..a743e1b 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -55,7 +55,7 @@ static unsigned int refresh_rate;
 #undef XGIFBDEBUG
 
 #ifdef XGIFBDEBUG
-#define DPRINTK(fmt, args...) printk(KERN_DEBUG "%s: " fmt, __FUNCTION__ , ## args)
+#define DPRINTK(fmt, args...) printk(KERN_DEBUG "%s: " fmt, __func__ , ## args)
 #else
 #define DPRINTK(fmt, args...)
 #endif
-- 
1.7.5.4

[PATCH 3/3] xgifb: checkpatch cleanup, printk() KERN_*

[Sam Hansen]

Added KERN_ facility levels in XGI_main_26.c and vb_init.c in a few different
printk() statements.

Signed-off-by: Sam Hansen <solid.se7en@gmail.com>
---
 drivers/staging/xgifb/XGI_main_26.c |   16 ++++++++--------
 drivers/staging/xgifb/vb_init.c     |    2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
index a743e1b..389768d 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -1711,7 +1711,7 @@ static int XGIfb_get_dram_size(struct xgifb_video_info *xgifb_info)
 	/* xgifb_info->video_size = 0x200000; */ /* 1024x768x16 */
 	/* xgifb_info->video_size = 0x1000000; */ /* benchmark */
 
-	printk("XGIfb: SR14=%x DramSzie %x ChannelNum %x\n",
+	printk(KERN_INFO "XGIfb: SR14=%x DramSzie %x ChannelNum %x\n",
 	       reg,
 	       xgifb_info->video_size, ChannelNum);
 	return 0;
@@ -1917,7 +1917,7 @@ static int __devinit xgifb_probe(struct pci_dev *pdev,
 	xgifb_info->vga_base = pci_resource_start(pdev, 2) + 0x30;
 	hw_info->pjIOAddress = (unsigned char *)xgifb_info->vga_base;
 	/* XGI_Pr.RelIO  = ioremap(pci_resource_start(pdev, 2), 128) + 0x30; */
-	printk("XGIfb: Relocate IO address: %lx [%08lx]\n",
+	printk(KERN_INFO "XGIfb: Relocate IO address: %lx [%08lx]\n",
 	       (unsigned long)pci_resource_start(pdev, 2),
 	       xgifb_info->dev_info.RelIO);
 
@@ -1937,7 +1937,7 @@ static int __devinit xgifb_probe(struct pci_dev *pdev,
 	reg1 = xgifb_reg_get(XGISR, IND_XGI_PASSWORD);
 
 	if (reg1 != 0xa1) { /*I/O error */
-		printk("\nXGIfb: I/O error!!!");
+		printk(KERN_ERR "\nXGIfb: I/O error!!!");
 		ret = -EIO;
 		goto error;
 	}
@@ -1989,7 +1989,7 @@ static int __devinit xgifb_probe(struct pci_dev *pdev,
 	if (!request_mem_region(xgifb_info->video_base,
 				xgifb_info->video_size,
 				"XGIfb FB")) {
-		printk("unable request memory size %x",
+		printk(KERN_ERR "unable to request memory size %x",
 		       xgifb_info->video_size);
 		printk(KERN_ERR "XGIfb: Fatal error: Unable to reserve frame buffer memory\n");
 		printk(KERN_ERR "XGIfb: Is there another framebuffer driver active?\n");
@@ -2018,12 +2018,12 @@ static int __devinit xgifb_probe(struct pci_dev *pdev,
 	printk(KERN_INFO "XGIfb: MMIO at 0x%lx, mapped to 0x%p, size %ldk\n",
 	       xgifb_info->mmio_base, xgifb_info->mmio_vbase,
 	       xgifb_info->mmio_size / 1024);
-	printk("XGIfb: XGIInitNew() ...");
+	printk(KERN_INFO "XGIfb: XGIInitNew() ...");
 	pci_set_drvdata(pdev, xgifb_info);
 	if (XGIInitNew(pdev))
-		printk("OK\n");
+		printk(KERN_INFO "OK\n");
 	else
-		printk("Fail\n");
+		printk(KERN_ERR "Fail\n");
 
 	xgifb_info->mtrr = (unsigned int) 0;
 
@@ -2064,7 +2064,7 @@ static int __devinit xgifb_probe(struct pci_dev *pdev,
 		} */
 		else {
 			hw_info->ujVBChipID = VB_CHIP_301;
-			printk("XGIfb: XGI301 bridge detected\n");
+			printk(KERN_INFO "XGIfb: XGI301 bridge detected\n");
 		}
 		break;
 	case HASVB_302:
diff --git a/drivers/staging/xgifb/vb_init.c b/drivers/staging/xgifb/vb_init.c
index 4ccd988..2b72e26 100644
--- a/drivers/staging/xgifb/vb_init.c
+++ b/drivers/staging/xgifb/vb_init.c
@@ -1483,7 +1483,7 @@ unsigned char XGIInitNew(struct pci_dev *pdev)
 	/* Newdebugcode(0x99); */
 
 	if (pVBInfo->FBAddr == NULL) {
-		printk("\n pVBInfo->FBAddr == 0 ");
+		printk(KERN_INFO "\n pVBInfo->FBAddr == 0 ");
 		return 0;
 	}
 	printk("1");
-- 
1.7.5.4

Re: [PATCH 3/3] xgifb: checkpatch cleanup, printk() KERN_*

[Dan Carpenter]

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

On Sat, Jan 21, 2012 at 02:10:12AM -0800, Sam Hansen wrote:
>  	if (reg1 != 0xa1) { /*I/O error */
> -		printk("\nXGIfb: I/O error!!!");
> +		printk(KERN_ERR "\nXGIfb: I/O error!!!");

It doesn't make sense to do this.  KERN_ERR puts a "<3>" in front of
the line so we know how important it is, but it's a blank line
because of the "\n" at the start.

Also the should probably be using pr_err() or dev_err().

> -	printk("XGIfb: XGIInitNew() ...");
> +	printk(KERN_INFO "XGIfb: XGIInitNew() ...");
>  	pci_set_drvdata(pdev, xgifb_info);
>  	if (XGIInitNew(pdev))
> -		printk("OK\n");
> +		printk(KERN_INFO "OK\n");
>  	else
> -		printk("Fail\n");
> +		printk(KERN_ERR "Fail\n");

These put a "<3>" in the middle of the line which doesn't help
anyone.  Rewrite it so it's on one line.  Get rid of the OK line.

>  	if (pVBInfo->FBAddr == NULL) {
> -		printk("\n pVBInfo->FBAddr == 0 ");
> +		printk(KERN_INFO "\n pVBInfo->FBAddr == 0 ");

Same.

regards,
dan carpenter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] xgifb: checkpatch cleanup, printk() KERN_*

[Joe Perches]

On Sat, 2012-01-21 at 02:10 -0800, Sam Hansen wrote:
> Added KERN_ facility levels in XGI_main_26.c and vb_init.c in a few different
> printk() statements.
[]
> @@ -2018,12 +2018,12 @@ static int __devinit xgifb_probe(struct pci_dev *pdev,
>  	printk(KERN_INFO "XGIfb: MMIO at 0x%lx, mapped to 0x%p, size %ldk\n",
>  	       xgifb_info->mmio_base, xgifb_info->mmio_vbase,
>  	       xgifb_info->mmio_size / 1024);
> -	printk("XGIfb: XGIInitNew() ...");
> +	printk(KERN_INFO "XGIfb: XGIInitNew() ...");
>  	pci_set_drvdata(pdev, xgifb_info);
>  	if (XGIInitNew(pdev))
> -		printk("OK\n");
> +		printk(KERN_INFO "OK\n");
>  	else
> -		printk("Fail\n");
> +		printk(KERN_ERR "Fail\n");

These last two should be KERN_CONT
though the block could be rewritten as

	if (XGIInitNew(pdev))
		printk(KERN_INFO "XGIfb: XGIInitNew() ... OK\n");
 	else
		printk(KERN_ERR "XGIfb: XGIInitNew() ... Fail\n");

Emitting any dmesg output like "XGIInitNew() ... OK\n"
is almost always low value.

Perhaps just emitting on the error case is better:
	if (!XGIInitNew(pdev))
		printk(KERN_ERR "XGIfb: XBIInitNew() failed\n");

One other thing.  It would be better to add
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
before any #include and convert all the
printks(KERN_<LEVEL> to pr_<level>( 
stripping all the leading XGIfb: too.

Re: [PATCH 3/3] xgifb: checkpatch cleanup, printk() KERN_*

[sam hansen]

> It doesn't make sense to do this.  KERN_ERR puts a "<3>" in front of
> the line so we know how important it is, but it's a blank line
> because of the "\n" at the start.
>
> Also the should probably be using pr_err() or dev_err().
>
> ...
>
> These put a "<3>" in the middle of the line which doesn't help
> anyone.  Rewrite it so it's on one line.  Get rid of the OK line.

Ah yes.  I will fold things up so that printk isn't adding level
messages in the middle of the line.  In the case where the dmesg line
is incrementally being built up, I may patch the generating logic so
everything is made from a single call to printk.

Thanks for the feedback.  I am just picking this up and this is my 2nd
patch (set?) ever :)

~s

Re: [PATCH 3/3] xgifb: checkpatch cleanup, printk() KERN_*

[sam hansen]

> These last two should be KERN_CONT
> though the block could be rewritten as
>
>        if (XGIInitNew(pdev))
>                printk(KERN_INFO "XGIfb: XGIInitNew() ... OK\n");
>        else
>                printk(KERN_ERR "XGIfb: XGIInitNew() ... Fail\n");
>
> Emitting any dmesg output like "XGIInitNew() ... OK\n"
> is almost always low value.
>
> Perhaps just emitting on the error case is better:
>        if (!XGIInitNew(pdev))
>                printk(KERN_ERR "XGIfb: XBIInitNew() failed\n");

Cool.  See Re: Dan.  I will fold the printk logic up.

> One other thing.  It would be better to add
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> before any #include and convert all the
> printks(KERN_<LEVEL> to pr_<level>(
> stripping all the leading XGIfb: too.

I might take a stab at eliminating explicit calls to printk and use a macro.

Re: [PATCH 3/3] xgifb: checkpatch cleanup, printk() KERN_*

[sam hansen]

> One other thing.  It would be better to add
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> before any #include and convert all the
> printks(KERN_<LEVEL> to pr_<level>(
> stripping all the leading XGIfb: too.

So, an administrative question.  Sorry for my noob-ishness but... I
will be 1) modifying the patch regarding prink to use pr_LVL (and a
few logic tweaks), and will also be introducing a new patch to set up
the pr_fmt macro for kbuild.  I can find lots of information regarding
how to create/submit a patch, but not a lot of information about how
to modify/resubmit a patch you've already submitted and received
feedback for.

The pr_fmt patch will likely just be a new patch outside of this patch
set?  Or should I use git-rebase to work it in the middle of the patch
set and regenerate/resend all patches (this approach doesn't seem to
make sense).

I'm not sure how the git commit short-hashes work with the patches,
and I don't want to destroy that information because it seems like
it's important when stuff goes upstream?

Thanks for any feedback you guys may have :)

And thanks for helping me pull myself up to actually being useful to you all.

~s

Re: [PATCH 3/3] xgifb: checkpatch cleanup, printk() KERN_*

[Joe Perches]

On Sat, 2012-01-21 at 11:22 -0800, sam hansen wrote:
> > One other thing.  It would be better to add
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > before any #include and convert all the
> > printks(KERN_<LEVEL> to pr_<level>(
> > stripping all the leading XGIfb: too.
> 
> So, an administrative question.  Sorry for my noob-ishness but... I
> will be 1) modifying the patch regarding prink to use pr_LVL (and a
> few logic tweaks), and will also be introducing a new patch to set up
> the pr_fmt macro for kbuild.  I can find lots of information regarding
> how to create/submit a patch, but not a lot of information about how
> to modify/resubmit a patch you've already submitted and received
> feedback for.

Rework the commented things in a new branch.

Apply the patches to the new branch one at
a time, modify as necessary then
git commit --amend
the appropriately changed files while modifying
change logs.

Then resubmit with
	git format-patch --subject-prefix="PATCH V2"
You could also add --in-reply-to=originalemailid

> The pr_fmt patch will likely just be a new patch outside of this patch
> set? 

A new patch would be fine.
Here's a little regex based tool for part of that conversion
http://lwn.net/Articles/380161/

An example would be:
$ ./scripts/cvt_kernel_style.pl			\
	--convert=convert_printk_to_pr_level	\
	-o drivers/staging/xgifb/XGI_main_26.c