[PATCH 01/10] staging/xgifb: Simplify XGI_GetRatePtrCRT2

[PATCH 01/10] staging/xgifb: Simplify XGI_GetRatePtrCRT2

[Peter Huewe]

Since the smaller LCDRefreshIndex is contained identically in LCDARefreshIndex
we can simply use LCDARefreshIndex and skip the if/else.

Since LCDARefreshIndex is only used readonly and contains only small
unsigned values we also change its declaration to const u8.

In order to prevent an out-of-bounds access I changed the mask from 0x0F
to 0x07 and added a dummy value.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
Please apply this series after my previous one 
 [PATCH 1/6] staging/xgifb: Remove unused variables and dead assignments
 [PATCH 2/6] staging/xgifb: Remove unused variable
 [PATCH 3/6] staging/xgifb: Remove unused variable and dead assignment
 [PATCH 4/6] staging/xgifb: Remove unused variables
 [PATCH 5/6] staging/xgifb: Remove redundant if statement
 [PATCH 6/6] staging/xgifb: remove unused variables

 drivers/staging/xgifb/vb_setmode.c |   16 ++++------------
 1 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/vb_setmode.c
index 8eb23a4..02e76c5 100644
--- a/drivers/staging/xgifb/vb_setmode.c
+++ b/drivers/staging/xgifb/vb_setmode.c
@@ -5470,9 +5470,8 @@ unsigned short XGI_GetRatePtrCRT2(struct xgi_hw_device_info *pXGIHWDE,
 		unsigned short ModeNo, unsigned short ModeIdIndex,
 		struct vb_device_info *pVBInfo)
 {
-	short LCDRefreshIndex[] = { 0x00, 0x00, 0x03, 0x01 },
-			LCDARefreshIndex[] = { 0x00, 0x00, 0x03, 0x01, 0x01,
-					0x01, 0x01 };
+	const u8 LCDARefreshIndex[] = {
+		0x00, 0x00, 0x03, 0x01, 0x01, 0x01, 0x01, 0x00 };
 
 	unsigned short RefreshRateTableIndex, i, index, temp;
 
@@ -5489,15 +5488,8 @@ unsigned short XGI_GetRatePtrCRT2(struct xgi_hw_device_info *pXGIHWDE,
 	if (pVBInfo->SetFlag & ProgrammingCRT2) {
 		if (pVBInfo->VBInfo & (SetCRT2ToLCD | XGI_SetCRT2ToLCDA)) {
 			if (pVBInfo->IF_DEF_LVDS == 0) {
-				if (pVBInfo->VBType & (VB_SIS301B | VB_SIS302B
-						| VB_SIS301LV | VB_SIS302LV
-						| VB_XGI301C))
-					/* 301b */
-					temp = LCDARefreshIndex[
-						pVBInfo->LCDResInfo & 0x0F];
-				else
-					temp = LCDRefreshIndex[
-						pVBInfo->LCDResInfo & 0x0F];
+				temp = LCDARefreshIndex[
+					pVBInfo->LCDResInfo & 0x07];
 
 				if (index > temp)
 					index = temp;
-- 
1.7.8.6

[PATCH 02/10] staging/xgifb: Remove always false comparisons

[Peter Huewe]

This patch removes some comparisons that always evaluate to false since
xoffset and yoffset are defined as __u32 in fb_var_screeninfo in
include/linux/fb.h and thus can never be negative.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/staging/xgifb/XGI_main_26.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
index ca9f583..83f8a6a 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -1333,12 +1333,6 @@ static int XGIfb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
 	/* Adapt RGB settings */
 	XGIfb_bpp_to_var(xgifb_info, var);
 
-	/* Sanity check for offsets */
-	if (var->xoffset < 0)
-		var->xoffset = 0;
-	if (var->yoffset < 0)
-		var->yoffset = 0;
-
 	if (!XGIfb_ypan) {
 		if (var->xres != var->xres_virtual)
 			var->xres_virtual = var->xres;
@@ -1373,8 +1367,7 @@ static int XGIfb_pan_display(struct fb_var_screeninfo *var,
 		return -EINVAL;
 
 	if (var->vmode & FB_VMODE_YWRAP) {
-		if (var->yoffset < 0 || var->yoffset >= info->var.yres_virtual
-				|| var->xoffset)
+		if (var->yoffset >= info->var.yres_virtual || var->xoffset)
 			return -EINVAL;
 	} else if (var->xoffset + info->var.xres > info->var.xres_virtual
 				|| var->yoffset + info->var.yres
-- 
1.7.8.6

[PATCH 03/10] staging/xgifb: mttr must be (signed) int

[Peter Huewe]

The mttr field must be declared as signed int (as in every other fb
driver) for the mttr functions to work properly.
Moreover the value should be initialized with -1.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/staging/xgifb/XGI_main_26.c |    2 +-
 drivers/staging/xgifb/XGIfb.h       |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
index 83f8a6a..fa351f9 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -1802,7 +1802,7 @@ static int xgifb_probe(struct pci_dev *pdev,
 	if (!XGIInitNew(pdev))
 		dev_err(&pdev->dev, "XGIInitNew() failed!\n");
 
-	xgifb_info->mtrr = (unsigned int) 0;
+	xgifb_info->mtrr = -1;
 
 	xgifb_info->hasVB = HASVB_NONE;
 	if ((xgifb_info->chip == XG20) ||
diff --git a/drivers/staging/xgifb/XGIfb.h b/drivers/staging/xgifb/XGIfb.h
index 8054798..af50362 100644
--- a/drivers/staging/xgifb/XGIfb.h
+++ b/drivers/staging/xgifb/XGIfb.h
@@ -67,7 +67,7 @@ struct xgifb_video_info {
 	unsigned long mmio_size;
 	void __iomem *mmio_vbase;
 	unsigned long vga_base;
-	unsigned long mtrr;
+	int mtrr;
 
 	int    video_bpp;
 	int    video_cmap_len;
-- 
1.7.8.6

[PATCH 04/10] staging/xgifb: Fix return of uninitialized variable

[Peter Huewe]

Clang warning:
drivers/staging/xgifb/XGI_main_26.c: warning: variable 'ret' is
used uninitialized whenever 'if' condition:
	if (xgifb_info->mode_idx < 0) {
evaluates to true.

drivers/staging/xgifb/XGI_main_26.c:
note: uninitialized use occurs here
	return ret;

This patch initializes the variable in this case.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/staging/xgifb/XGI_main_26.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
index fa351f9..106abc8 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -1921,6 +1921,7 @@ static int xgifb_probe(struct pci_dev *pdev,
 
 	if (xgifb_info->mode_idx < 0) {
 		dev_err(&pdev->dev, "No supported video mode found\n");
+		ret = -EINVAL;
 		goto error_1;
 	}
 
-- 
1.7.8.6

[PATCH 05/10] staging/xgifb: Simplify XGI_SetSeqRegs

[Peter Huewe]

Since SR[0] in the (readonly) XGI330_StandTable is always 0x01 we can
skip or'ing with 0x01 and make the code simpler by removing the if
statements.

Since this function is the only user of the XGI330_StandTable we can
also include the unconditional |= 0x20 into the input data and move the
assignment to SR1 into the loop, which I prefer to start at 0.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/staging/xgifb/vb_setmode.c |   23 ++++++-----------------
 drivers/staging/xgifb/vb_table.h   |    2 +-
 2 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/vb_setmode.c
index 02e76c5..6f366f4 100644
--- a/drivers/staging/xgifb/vb_setmode.c
+++ b/drivers/staging/xgifb/vb_setmode.c
@@ -64,26 +64,15 @@ static void XGI_SetSeqRegs(unsigned short ModeNo,
 			   unsigned short ModeIdIndex,
 			   struct vb_device_info *pVBInfo)
 {
-	unsigned char tempah, SRdata;
-	unsigned short i;
+	unsigned char SRdata, i;
 
 	xgifb_reg_set(pVBInfo->P3c4, 0x00, 0x03); /* Set SR0 */
-	tempah = XGI330_StandTable.SR[0];
-
-	if (pVBInfo->VBInfo & XGI_SetCRT2ToLCDA) {
-		tempah |= 0x01;
-	} else if (pVBInfo->VBInfo & (SetCRT2ToTV | SetCRT2ToLCD)) {
-		if (pVBInfo->VBInfo & SetInSlaveMode)
-			tempah |= 0x01;
-	}
 
-	tempah |= 0x20; /* screen off */
-	xgifb_reg_set(pVBInfo->P3c4, 0x01, tempah); /* Set SR1 */
-
-	for (i = 02; i <= 04; i++) {
-		/* Get SR2,3,4 from file */
-		SRdata = XGI330_StandTable.SR[i - 1];
-		xgifb_reg_set(pVBInfo->P3c4, i, SRdata); /* Set SR2 3 4 */
+	for (i = 0; i < 4; i++) {
+		/* Get SR1,2,3,4 from file */
+		/* SR1 is with screen off 0x20 */
+		SRdata = XGI330_StandTable.SR[i];
+		xgifb_reg_set(pVBInfo->P3c4, i+1, SRdata); /* Set SR 1 2 3 4 */
 	}
 }
 
diff --git a/drivers/staging/xgifb/vb_table.h b/drivers/staging/xgifb/vb_table.h
index 39f528b..fca1a1d 100644
--- a/drivers/staging/xgifb/vb_table.h
+++ b/drivers/staging/xgifb/vb_table.h
@@ -195,7 +195,7 @@ const struct XGI_ExtStruct XGI330_EModeIDTable[] = {
 static const struct SiS_StandTable_S XGI330_StandTable = {
 /* ExtVGATable */
 	0x00, 0x00, 0x00, 0x0000,
-	{0x01, 0x0f, 0x00, 0x0e},
+	{0x21, 0x0f, 0x00, 0x0e}, /* 0x21 = 0x01 | (0x20 = screen off) */
 	 0x23,
 	{0x5f, 0x4f, 0x50, 0x82, 0x54, 0x80, 0x0b, 0x3e,
 	 0x00, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-- 
1.7.8.6

[PATCH 06/10] staging/xgifb: rewrite XGIfb_get_cmap_len

[Peter Huewe]

We don't need to use this switch-case here for a simple two case
if-else.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/staging/xgifb/XGI_main_26.c |   16 ++--------------
 1 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
index 106abc8..840b497 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -1131,22 +1131,10 @@ static int XGIfb_release(struct fb_info *info, int user)
 	return 0;
 }
 
+/* similar to sisfb_get_cmap_len */
 static int XGIfb_get_cmap_len(const struct fb_var_screeninfo *var)
 {
-	int rc = 16;
-
-	switch (var->bits_per_pixel) {
-	case 8:
-		rc = 256;
-		break;
-	case 16:
-		rc = 16;
-		break;
-	case 32:
-		rc = 16;
-		break;
-	}
-	return rc;
+	return (var->bits_per_pixel == 8) ? 256 : 16;
 }
 
 static int XGIfb_setcolreg(unsigned regno, unsigned red, unsigned green,
-- 
1.7.8.6

[PATCH 07/10] staging/xgifb: remove unnecessary temp variable in XGIfb_mode_rate_to_ddata

[Peter Huewe]

Instead of subtracting one and then assign a different name and add 1
again we simply use HDE directly. HDE wasn't used directly before, so no
change in functionality.
Same applies to VDE.

-> now we can remove the variable with the very descriptive name E ;)

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/staging/xgifb/XGI_main_26.c |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
index 840b497..387d1bd 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -94,7 +94,7 @@ static int XGIfb_mode_rate_to_ddata(struct vb_device_info *XGI_Pr,
 	unsigned short VRE, VBE, VRS, VDE;
 	unsigned short HRE, HBE, HRS, HDE;
 	unsigned char sr_data, cr_data, cr_data2;
-	int B, C, D, E, F, temp, j;
+	int B, C, D, F, temp, j;
 	InitTo330Pointer(HwDeviceExtension->jChipType, XGI_Pr);
 	if (!XGI_SearchModeID(ModeNo, &ModeIdIndex, XGI_Pr))
 		return 0;
@@ -104,14 +104,13 @@ static int XGIfb_mode_rate_to_ddata(struct vb_device_info *XGI_Pr,
 
 	sr_data = XGI_CRT1Table[index].CR[5];
 
-	HDE = (XGI330_RefIndex[RefreshRateTableIndex].XRes >> 3) - 1;
-	E = HDE + 1;
+	HDE = (XGI330_RefIndex[RefreshRateTableIndex].XRes >> 3);
 
 	cr_data = XGI_CRT1Table[index].CR[3];
 
 	/* Horizontal retrace (=sync) start */
 	HRS = (cr_data & 0xff) | ((unsigned short) (sr_data & 0xC0) << 2);
-	F = HRS - E - 3;
+	F = HRS - HDE - 3;
 
 	sr_data = XGI_CRT1Table[index].CR[6];
 
@@ -126,10 +125,10 @@ static int XGIfb_mode_rate_to_ddata(struct vb_device_info *XGI_Pr,
 	/* Horizontal retrace (=sync) end */
 	HRE = (cr_data2 & 0x1f) | ((sr_data & 0x04) << 3);
 
-	temp = HBE - ((E - 1) & 255);
+	temp = HBE - ((HDE - 1) & 255);
 	B = (temp > 0) ? temp : (temp + 256);
 
-	temp = HRE - ((E + F + 3) & 63);
+	temp = HRE - ((HDE + F + 3) & 63);
 	C = (temp > 0) ? temp : (temp + 64);
 
 	D = B - F - C;
@@ -142,8 +141,7 @@ static int XGIfb_mode_rate_to_ddata(struct vb_device_info *XGI_Pr,
 
 	cr_data2 = XGI_CRT1Table[index].CR[9];
 
-	VDE = XGI330_RefIndex[RefreshRateTableIndex].YRes - 1;
-	E = VDE + 1;
+	VDE = XGI330_RefIndex[RefreshRateTableIndex].YRes;
 
 	cr_data = XGI_CRT1Table[index].CR[10];
 
@@ -151,20 +149,20 @@ static int XGIfb_mode_rate_to_ddata(struct vb_device_info *XGI_Pr,
 	VRS = (cr_data & 0xff) | ((unsigned short) (cr_data2 & 0x04) << 6)
 			| ((unsigned short) (cr_data2 & 0x80) << 2)
 			| ((unsigned short) (sr_data & 0x08) << 7);
-	F = VRS + 1 - E;
+	F = VRS + 1 - VDE;
 
 	cr_data = XGI_CRT1Table[index].CR[13];
 
 	/* Vertical blank end */
 	VBE = (cr_data & 0xff) | ((unsigned short) (sr_data & 0x10) << 4);
-	temp = VBE - ((E - 1) & 511);
+	temp = VBE - ((VDE - 1) & 511);
 	B = (temp > 0) ? temp : (temp + 512);
 
 	cr_data = XGI_CRT1Table[index].CR[11];
 
 	/* Vertical retrace (=sync) end */
 	VRE = (cr_data & 0x0f) | ((sr_data & 0x20) >> 1);
-	temp = VRE - ((E + F - 1) & 31);
+	temp = VRE - ((VDE + F - 1) & 31);
 	C = (temp > 0) ? temp : (temp + 32);
 
 	D = B - F - C;
-- 
1.7.8.6

[PATCH 08/10] staging/xgifb: Remove unnecessary bitshifts in XGI_SetCRT1ModeRegs

[Peter Huewe]

Since data can only be 0x0000, 0x0035 or 0x0048 we can simply skip the
bit shifting and masking as data & 0xFF is always equal to data and
data & 0xFF00 is always 0.
So we simply use data and 0 directly and save the assignment.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/staging/xgifb/vb_setmode.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/vb_setmode.c
index 6f366f4..1ff1178 100644
--- a/drivers/staging/xgifb/vb_setmode.c
+++ b/drivers/staging/xgifb/vb_setmode.c
@@ -1083,10 +1083,8 @@ static void XGI_SetCRT1ModeRegs(struct xgi_hw_device_info *HwDeviceExtension,
 			data = 0x0048;
 	}
 
-	data2 = data & 0x00FF;
-	xgifb_reg_and_or(pVBInfo->P3d4, 0x19, 0xFF, data2);
-	data2 = (data & 0xFF00) >> 8;
-	xgifb_reg_and_or(pVBInfo->P3d4, 0x19, 0xFC, data2);
+	xgifb_reg_and_or(pVBInfo->P3d4, 0x19, 0xFF, data);
+	xgifb_reg_and_or(pVBInfo->P3d4, 0x19, 0xFC, 0);
 
 	if (modeflag & HalfDCLK)
 		xgifb_reg_and_or(pVBInfo->P3c4, 0x01, 0xF7, 0x08);
-- 
1.7.8.6

[PATCH 09/10] staging/xgifb: Consolidate XGI_EnableChISLCD and XGI_DisableChISLCD

[Peter Huewe]

These two functions share the same code except one line - thus we can
simply merge them and add a parameter to switch between both variants.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/staging/xgifb/vb_setmode.c |   48 +++++++++--------------------------
 1 files changed, 13 insertions(+), 35 deletions(-)

diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/vb_setmode.c
index 1ff1178..c5faa89 100644
--- a/drivers/staging/xgifb/vb_setmode.c
+++ b/drivers/staging/xgifb/vb_setmode.c
@@ -4684,43 +4684,21 @@ static unsigned char XGI_IsLCDON(struct vb_device_info *pVBInfo)
 }
 
 /* --------------------------------------------------------------------- */
-/* Function : XGI_DisableChISLCD */
-/* Input : */
-/* Output : 0 -> Not LCD Mode */
-/* Description : */
-/* --------------------------------------------------------------------- */
-static unsigned char XGI_DisableChISLCD(struct vb_device_info *pVBInfo)
-{
-	unsigned short tempbx, tempah;
-
-	tempbx = pVBInfo->SetFlag & (DisableChA | DisableChB);
-	tempah = ~((unsigned short) xgifb_reg_get(pVBInfo->Part1Port, 0x2E));
-
-	if (tempbx & (EnableChA | DisableChA)) {
-		if (!(tempah & 0x08)) /* Chk LCDA Mode */
-			return 0;
-	}
-
-	if (!(tempbx & (EnableChB | DisableChB)))
-		return 0;
-
-	if (tempah & 0x01) /* Chk LCDB Mode */
-		return 1;
-
-	return 0;
-}
-
-/* --------------------------------------------------------------------- */
 /* Function : XGI_EnableChISLCD */
 /* Input : */
 /* Output : 0 -> Not LCD mode */
-/* Description : */
+/* Description : if bool enable = true -> enable, else disable  */
 /* --------------------------------------------------------------------- */
-static unsigned char XGI_EnableChISLCD(struct vb_device_info *pVBInfo)
+static unsigned char XGI_EnableChISLCD(struct vb_device_info *pVBInfo,
+	bool enable)
 {
 	unsigned short tempbx, tempah;
 
-	tempbx = pVBInfo->SetFlag & (EnableChA | EnableChB);
+	if (enable)
+		tempbx = pVBInfo->SetFlag & (EnableChA | EnableChB);
+	else
+		tempbx = pVBInfo->SetFlag & (DisableChA | DisableChB);
+
 	tempah = ~((unsigned short) xgifb_reg_get(pVBInfo->Part1Port, 0x2E));
 
 	if (tempbx & (EnableChA | DisableChA)) {
@@ -4772,9 +4750,9 @@ static void XGI_DisableBridge(struct xgifb_video_info *xgifb_info,
 
 		if (pVBInfo->VBType & (VB_SIS302LV | VB_XGI301C)) {
 			if (((pVBInfo->VBInfo &
-			      (SetCRT2ToLCD | XGI_SetCRT2ToLCDA)))
-			    || (XGI_DisableChISLCD(pVBInfo))
-			    || (XGI_IsLCDON(pVBInfo)))
+			      (SetCRT2ToLCD | XGI_SetCRT2ToLCDA))) ||
+				(XGI_EnableChISLCD(pVBInfo, false)) ||
+				(XGI_IsLCDON(pVBInfo)))
 				/* LVDS Driver power down */
 				xgifb_reg_or(pVBInfo->Part4Port, 0x30, 0x80);
 		}
@@ -5731,8 +5709,8 @@ static void XGI_EnableBridge(struct xgifb_video_info *xgifb_info,
 			xgifb_reg_and_or(pVBInfo->Part2Port, 0x00, ~0xE0,
 					0x20); /* shampoo 0129 */
 			if (pVBInfo->VBType & (VB_SIS302LV | VB_XGI301C)) {
-				if (!XGI_DisableChISLCD(pVBInfo)) {
-					if (XGI_EnableChISLCD(pVBInfo) ||
+				if (!XGI_EnableChISLCD(pVBInfo, false)) {
+					if (XGI_EnableChISLCD(pVBInfo, true) ||
 					    (pVBInfo->VBInfo &
 					    (SetCRT2ToLCD | XGI_SetCRT2ToLCDA)))
 						/* LVDS PLL power on */
-- 
1.7.8.6

[PATCH 10/10] staging/xgifb: Simplify XGISetModeNew

[Peter Huewe]

This patch simplifies the code of XGISetModeNew by reordering the
if/else if/case conditions when both branches are doing exactly the
same.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/staging/xgifb/vb_setmode.c |   18 +++---------------
 1 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/vb_setmode.c
index c5faa89..f44a254 100644
--- a/drivers/staging/xgifb/vb_setmode.c
+++ b/drivers/staging/xgifb/vb_setmode.c
@@ -5907,7 +5907,8 @@ unsigned char XGISetModeNew(struct xgifb_video_info *xgifb_info,
 		XGI_GetLCDInfo(ModeNo, ModeIdIndex, pVBInfo);
 		XGI_DisableBridge(xgifb_info, HwDeviceExtension, pVBInfo);
 
-		if (pVBInfo->VBInfo & (SetSimuScanMode | XGI_SetCRT2ToLCDA)) {
+		if (pVBInfo->VBInfo & (SetSimuScanMode | XGI_SetCRT2ToLCDA) ||
+			(!(pVBInfo->VBInfo & SwitchCRT2))) {
 			XGI_SetCRT1Group(xgifb_info, HwDeviceExtension, ModeNo,
 					ModeIdIndex, pVBInfo);
 
@@ -5915,24 +5916,11 @@ unsigned char XGISetModeNew(struct xgifb_video_info *xgifb_info,
 				XGI_SetLCDAGroup(ModeNo, ModeIdIndex,
 						HwDeviceExtension, pVBInfo);
 			}
-		} else if (!(pVBInfo->VBInfo & SwitchCRT2)) {
-			XGI_SetCRT1Group(xgifb_info,
-					HwDeviceExtension, ModeNo,
-					ModeIdIndex, pVBInfo);
-			if (pVBInfo->VBInfo & XGI_SetCRT2ToLCDA) {
-				XGI_SetLCDAGroup(ModeNo, ModeIdIndex,
-						HwDeviceExtension,
-						pVBInfo);
-			}
 		}
 
 		if (pVBInfo->VBInfo & (SetSimuScanMode | SwitchCRT2)) {
 			switch (HwDeviceExtension->ujVBChipID) {
-			case VB_CHIP_301:
-				XGI_SetCRT2Group301(ModeNo, HwDeviceExtension,
-						pVBInfo); /*add for CRT2 */
-				break;
-
+			case VB_CHIP_301: /* fall through */
 			case VB_CHIP_302:
 				XGI_SetCRT2Group301(ModeNo, HwDeviceExtension,
 						pVBInfo); /*add for CRT2 */
-- 
1.7.8.6

Re: [PATCH 08/10] staging/xgifb: Remove unnecessary bitshifts in XGI_SetCRT1ModeRegs

[Dan Carpenter]

On Sun, Feb 03, 2013 at 10:54:37PM +0100, Peter Huewe wrote:
> Since data can only be 0x0000, 0x0035 or 0x0048 we can simply skip the
> bit shifting and masking as data & 0xFF is always equal to data and
> data & 0xFF00 is always 0.
> So we simply use data and 0 directly and save the assignment.
> 
> Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
> ---
>  drivers/staging/xgifb/vb_setmode.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/vb_setmode.c
> index 6f366f4..1ff1178 100644
> --- a/drivers/staging/xgifb/vb_setmode.c
> +++ b/drivers/staging/xgifb/vb_setmode.c
> @@ -1083,10 +1083,8 @@ static void XGI_SetCRT1ModeRegs(struct xgi_hw_device_info *HwDeviceExtension,
>  			data = 0x0048;
>  	}
>  
> -	data2 = data & 0x00FF;
> -	xgifb_reg_and_or(pVBInfo->P3d4, 0x19, 0xFF, data2);
> -	data2 = (data & 0xFF00) >> 8;
> -	xgifb_reg_and_or(pVBInfo->P3d4, 0x19, 0xFC, data2);
> +	xgifb_reg_and_or(pVBInfo->P3d4, 0x19, 0xFF, data);
> +	xgifb_reg_and_or(pVBInfo->P3d4, 0x19, 0xFC, 0);

This patch is fine.

This is a fairly common pattern where people do:

	write_a_char(data & 0xff);
	write_a_char((data >> 8) & 0xff);

It feels there should be a macro to do the shift and mask but I'm
not sure what it would look like.

regards,
dan carpenter

RE: [PATCH 08/10] staging/xgifb: Remove unnecessary bitshifts in XGI_SetCRT1ModeRegs

[H Hartley Sweeten]

On Monday, February 04, 2013 5:59 AM, Dan Carpenter wrote:
> On Sun, Feb 03, 2013 at 10:54:37PM +0100, Peter Huewe wrote:
>> Since data can only be 0x0000, 0x0035 or 0x0048 we can simply skip the
>> bit shifting and masking as data & 0xFF is always equal to data and
>> data & 0xFF00 is always 0.
>> So we simply use data and 0 directly and save the assignment.
>> 
>> Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
>> ---
>>  drivers/staging/xgifb/vb_setmode.c |    6 ++----
>>  1 files changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/staging/xgifb/vb_setmode.c b/drivers/staging/xgifb/vb_setmode.c
>> index 6f366f4..1ff1178 100644
>> --- a/drivers/staging/xgifb/vb_setmode.c
>> +++ b/drivers/staging/xgifb/vb_setmode.c
>> @@ -1083,10 +1083,8 @@ static void XGI_SetCRT1ModeRegs(struct xgi_hw_device_info *HwDeviceExtension,
>>  			data = 0x0048;
>>  	}
>>  
>> -	data2 = data & 0x00FF;
>> -	xgifb_reg_and_or(pVBInfo->P3d4, 0x19, 0xFF, data2);
>> -	data2 = (data & 0xFF00) >> 8;
>> -	xgifb_reg_and_or(pVBInfo->P3d4, 0x19, 0xFC, data2);
>> +	xgifb_reg_and_or(pVBInfo->P3d4, 0x19, 0xFF, data);
>> +	xgifb_reg_and_or(pVBInfo->P3d4, 0x19, 0xFC, 0);
>
> This patch is fine.
>
> This is a fairly common pattern where people do:
>
>	write_a_char(data & 0xff);
>	write_a_char((data >> 8) & 0xff);
>
> It feels there should be a macro to do the shift and mask but I'm
> not sure what it would look like.

How about...

#define LOBYTE(x)	((x) & 0xff)
#define HIBYTE(x)	(((x) >> 8) & 0xff)

And for completeness...

#define LOWORD	((x) & 0xffff)
#define HIWORD	(((x) >> 16) & 0xffff)

There are a couple uses of these defines in the kernel already.
sound/synth/emux/emux_synth.c also has LO_BYTE and HI_BYTE
that do the same thing.

Of course these are probably not endian safe.

Regards,
Hartley

Re: [PATCH 01/10] staging/xgifb: Simplify XGI_GetRatePtrCRT2

[Aaro Koskinen]

Hi,

On Sun, Feb 03, 2013 at 10:54:30PM +0100, Peter Huewe wrote:
> Since the smaller LCDRefreshIndex is contained identically in LCDARefreshIndex
> we can simply use LCDARefreshIndex and skip the if/else.
> 
> Since LCDARefreshIndex is only used readonly and contains only small
> unsigned values we also change its declaration to const u8.
> 
> In order to prevent an out-of-bounds access I changed the mask from 0x0F
> to 0x07 and added a dummy value.
> 
> Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
> ---
> Please apply this series after my previous one 
>  [PATCH 1/6] staging/xgifb: Remove unused variables and dead assignments
>  [PATCH 2/6] staging/xgifb: Remove unused variable
>  [PATCH 3/6] staging/xgifb: Remove unused variable and dead assignment
>  [PATCH 4/6] staging/xgifb: Remove unused variables
>  [PATCH 5/6] staging/xgifb: Remove redundant if statement
>  [PATCH 6/6] staging/xgifb: remove unused variables

I think both of these series are good. I boot tested them with Z9/XG21,
and at least fb console seems to work fine. I also checked compilation
for warnings with GCC+sparse and could not see any. So:

Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>

for all of the patches.

Thanks,

A.