* [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function
@ 2018-03-20 8:35 Pratik Jain
2018-03-20 11:20 ` Dan Carpenter
2018-03-20 12:03 ` Dan Carpenter
0 siblings, 2 replies; 10+ messages in thread
From: Pratik Jain @ 2018-03-20 8:35 UTC (permalink / raw)
To: arnaud.patard; +Cc: linux-kernel, devel, gregkh, Pratik Jain
Refactored the function `XGIfb_search_refresh_rate` by removing a level
of `if...else` block nesting. Removed unnecessary parantheses.
Signed-off-by: Pratik Jain <pratik.jain0509@gmail.com>
---
drivers/staging/xgifb/XGI_main_26.c | 63 +++++++++++++++++++------------------
1 file changed, 33 insertions(+), 30 deletions(-)
diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
index 10107de0119a..ef9a726cd35d 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -544,41 +544,44 @@ static u8 XGIfb_search_refresh_rate(struct xgifb_video_info *xgifb_info,
yres = XGIbios_mode[xgifb_info->mode_idx].yres;
xgifb_info->rate_idx = 0;
- while ((XGIfb_vrate[i].idx != 0) && (XGIfb_vrate[i].xres <= xres)) {
- if ((XGIfb_vrate[i].xres == xres) &&
- (XGIfb_vrate[i].yres == yres)) {
- if (XGIfb_vrate[i].refresh == rate) {
+
+ // Skip values with less xres
+ while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres < xres)
+ ++i;
+
+ while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres <= xres) {
+ if (XGIfb_vrate[i].yres != yres) {
+ ++i;
+ continue;
+ }
+ if (XGIfb_vrate[i].refresh == rate) {
+ xgifb_info->rate_idx = XGIfb_vrate[i].idx;
+ break;
+ } else if (XGIfb_vrate[i].refresh > rate) {
+ if (XGIfb_vrate[i].refresh - rate <= 3) {
+ pr_debug("Adjusting rate from %d up to %d\n",
+ rate, XGIfb_vrate[i].refresh);
xgifb_info->rate_idx = XGIfb_vrate[i].idx;
- break;
- } else if (XGIfb_vrate[i].refresh > rate) {
- if ((XGIfb_vrate[i].refresh - rate) <= 3) {
- pr_debug("Adjusting rate from %d up to %d\n",
- rate, XGIfb_vrate[i].refresh);
- xgifb_info->rate_idx =
- XGIfb_vrate[i].idx;
- xgifb_info->refresh_rate =
- XGIfb_vrate[i].refresh;
- } else if (((rate - XGIfb_vrate[i - 1].refresh)
- <= 2) && (XGIfb_vrate[i].idx
- != 1)) {
- pr_debug("Adjusting rate from %d down to %d\n",
- rate,
- XGIfb_vrate[i - 1].refresh);
- xgifb_info->rate_idx =
- XGIfb_vrate[i - 1].idx;
- xgifb_info->refresh_rate =
- XGIfb_vrate[i - 1].refresh;
- }
- break;
- } else if ((rate - XGIfb_vrate[i].refresh) <= 2) {
+ xgifb_info->refresh_rate =
+ XGIfb_vrate[i].refresh;
+ } else if ((rate - XGIfb_vrate[i - 1].refresh <= 2)
+ && (XGIfb_vrate[i].idx != 1)) {
pr_debug("Adjusting rate from %d down to %d\n",
- rate, XGIfb_vrate[i].refresh);
- xgifb_info->rate_idx = XGIfb_vrate[i].idx;
- break;
+ rate, XGIfb_vrate[i - 1].refresh);
+ xgifb_info->rate_idx = XGIfb_vrate[i - 1].idx;
+ xgifb_info->refresh_rate =
+ XGIfb_vrate[i - 1].refresh;
}
+ break;
+ } else if (rate - XGIfb_vrate[i].refresh <= 2) {
+ pr_debug("Adjusting rate from %d down to %d\n",
+ rate, XGIfb_vrate[i].refresh);
+ xgifb_info->rate_idx = XGIfb_vrate[i].idx;
+ break;
}
- i++;
+ ++i;
}
+
if (xgifb_info->rate_idx > 0)
return xgifb_info->rate_idx;
pr_info("Unsupported rate %d for %dx%d\n",
--
2.16.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function
2018-03-20 8:35 [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function Pratik Jain
@ 2018-03-20 11:20 ` Dan Carpenter
2018-03-20 11:41 ` Pratik Jain
2018-03-20 12:03 ` Dan Carpenter
1 sibling, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2018-03-20 11:20 UTC (permalink / raw)
To: Pratik Jain; +Cc: arnaud.patard, devel, gregkh, linux-kernel
I'm trying to review this, but I feel like this makes it slightly more
complicated for no reason. Why break it up into two loops?
> - i++;
> + ++i;
These are equivalent, so you should default to accepting the original
author's style. Otherwise the next person to touch this code will just
change it back and we get into a cycle of pointless changes.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function
2018-03-20 11:20 ` Dan Carpenter
@ 2018-03-20 11:41 ` Pratik Jain
2018-03-20 11:54 ` Pratik Jain
0 siblings, 1 reply; 10+ messages in thread
From: Pratik Jain @ 2018-03-20 11:41 UTC (permalink / raw)
To: Dan Carpenter; +Cc: arnaud.patard, devel, gregkh, linux-kernel
You got a valid point about `++i` and `i++`. But I still feel
that it is less complicated than previous one. I am explicitly
saying(in first loop) that we are skipping some values to get
to a specific index. Apart from that, we can avoid unncessary
wrapping and indentation. Logic becomes much more explicit if
broken down to two loops.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function
2018-03-20 11:41 ` Pratik Jain
@ 2018-03-20 11:54 ` Pratik Jain
0 siblings, 0 replies; 10+ messages in thread
From: Pratik Jain @ 2018-03-20 11:54 UTC (permalink / raw)
To: arnaud.patard; +Cc: linux-kernel, devel, gregkh, Pratik Jain
Refactored the function `XGIfb_search_refresh_rate` by removing a level
of `if...else` block nesting. Removed unnecessary parantheses.
Signed-off-by: Pratik Jain <pratik.jain0509@gmail.com>
---
drivers/staging/xgifb/XGI_main_26.c | 61 +++++++++++++++++++------------------
1 file changed, 32 insertions(+), 29 deletions(-)
diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
index 10107de0119a..7bbc12f3146f 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -544,41 +544,44 @@ static u8 XGIfb_search_refresh_rate(struct xgifb_video_info *xgifb_info,
yres = XGIbios_mode[xgifb_info->mode_idx].yres;
xgifb_info->rate_idx = 0;
- while ((XGIfb_vrate[i].idx != 0) && (XGIfb_vrate[i].xres <= xres)) {
- if ((XGIfb_vrate[i].xres == xres) &&
- (XGIfb_vrate[i].yres == yres)) {
- if (XGIfb_vrate[i].refresh == rate) {
+
+ // Skip values with less xres
+ while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres < xres)
+ i++;
+
+ while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres <= xres) {
+ if (XGIfb_vrate[i].yres != yres) {
+ i++;
+ continue;
+ }
+ if (XGIfb_vrate[i].refresh == rate) {
+ xgifb_info->rate_idx = XGIfb_vrate[i].idx;
+ break;
+ } else if (XGIfb_vrate[i].refresh > rate) {
+ if (XGIfb_vrate[i].refresh - rate <= 3) {
+ pr_debug("Adjusting rate from %d up to %d\n",
+ rate, XGIfb_vrate[i].refresh);
xgifb_info->rate_idx = XGIfb_vrate[i].idx;
- break;
- } else if (XGIfb_vrate[i].refresh > rate) {
- if ((XGIfb_vrate[i].refresh - rate) <= 3) {
- pr_debug("Adjusting rate from %d up to %d\n",
- rate, XGIfb_vrate[i].refresh);
- xgifb_info->rate_idx =
- XGIfb_vrate[i].idx;
- xgifb_info->refresh_rate =
- XGIfb_vrate[i].refresh;
- } else if (((rate - XGIfb_vrate[i - 1].refresh)
- <= 2) && (XGIfb_vrate[i].idx
- != 1)) {
- pr_debug("Adjusting rate from %d down to %d\n",
- rate,
- XGIfb_vrate[i - 1].refresh);
- xgifb_info->rate_idx =
- XGIfb_vrate[i - 1].idx;
- xgifb_info->refresh_rate =
- XGIfb_vrate[i - 1].refresh;
- }
- break;
- } else if ((rate - XGIfb_vrate[i].refresh) <= 2) {
+ xgifb_info->refresh_rate =
+ XGIfb_vrate[i].refresh;
+ } else if ((rate - XGIfb_vrate[i - 1].refresh <= 2)
+ && (XGIfb_vrate[i].idx != 1)) {
pr_debug("Adjusting rate from %d down to %d\n",
- rate, XGIfb_vrate[i].refresh);
- xgifb_info->rate_idx = XGIfb_vrate[i].idx;
- break;
+ rate, XGIfb_vrate[i - 1].refresh);
+ xgifb_info->rate_idx = XGIfb_vrate[i - 1].idx;
+ xgifb_info->refresh_rate =
+ XGIfb_vrate[i - 1].refresh;
}
+ break;
+ } else if (rate - XGIfb_vrate[i].refresh <= 2) {
+ pr_debug("Adjusting rate from %d down to %d\n",
+ rate, XGIfb_vrate[i].refresh);
+ xgifb_info->rate_idx = XGIfb_vrate[i].idx;
+ break;
}
i++;
}
+
if (xgifb_info->rate_idx > 0)
return xgifb_info->rate_idx;
pr_info("Unsupported rate %d for %dx%d\n",
--
2.16.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function
2018-03-20 8:35 [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function Pratik Jain
2018-03-20 11:20 ` Dan Carpenter
@ 2018-03-20 12:03 ` Dan Carpenter
2018-03-20 14:02 ` Pratik Jain
1 sibling, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2018-03-20 12:03 UTC (permalink / raw)
To: Pratik Jain; +Cc: devel, gregkh, linux-kernel, arnaud.patard
On Tue, Mar 20, 2018 at 02:05:49PM +0530, Pratik Jain wrote:
> Refactored the function `XGIfb_search_refresh_rate` by removing a level
> of `if...else` block nesting. Removed unnecessary parantheses.
>
> Signed-off-by: Pratik Jain <pratik.jain0509@gmail.com>
> ---
> drivers/staging/xgifb/XGI_main_26.c | 63 +++++++++++++++++++------------------
> 1 file changed, 33 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
> index 10107de0119a..ef9a726cd35d 100644
> --- a/drivers/staging/xgifb/XGI_main_26.c
> +++ b/drivers/staging/xgifb/XGI_main_26.c
> @@ -544,41 +544,44 @@ static u8 XGIfb_search_refresh_rate(struct xgifb_video_info *xgifb_info,
> yres = XGIbios_mode[xgifb_info->mode_idx].yres;
>
> xgifb_info->rate_idx = 0;
> - while ((XGIfb_vrate[i].idx != 0) && (XGIfb_vrate[i].xres <= xres)) {
> - if ((XGIfb_vrate[i].xres == xres) &&
> - (XGIfb_vrate[i].yres == yres)) {
> - if (XGIfb_vrate[i].refresh == rate) {
> +
There is a stray tab here. You didn't run checkpatch.pl.
> + // Skip values with less xres
Linus likes this comment style, but I would prefer normal comments,
please.
> + while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres < xres)
> + ++i;
> +
I have reviewed the code, and I still find the single loop more
readable.
> + while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres <= xres) {
> + if (XGIfb_vrate[i].yres != yres) {
> + ++i;
> + continue;
> + }
I would like a change that did:
if ((XGIfb_vrate[i].xres != xres) ||
(XGIfb_vrate[i].yres != yres)) {
i++;
continue;
}
so we could pull everything in one tab.
> + if (XGIfb_vrate[i].refresh == rate) {
> + xgifb_info->rate_idx = XGIfb_vrate[i].idx;
> + break;
> + } else if (XGIfb_vrate[i].refresh > rate) {
> + if (XGIfb_vrate[i].refresh - rate <= 3) {
> + pr_debug("Adjusting rate from %d up to %d\n",
> + rate, XGIfb_vrate[i].refresh);
> xgifb_info->rate_idx = XGIfb_vrate[i].idx;
> - break;
> - } else if (XGIfb_vrate[i].refresh > rate) {
> - if ((XGIfb_vrate[i].refresh - rate) <= 3) {
> - pr_debug("Adjusting rate from %d up to %d\n",
> - rate, XGIfb_vrate[i].refresh);
> - xgifb_info->rate_idx =
> - XGIfb_vrate[i].idx;
> - xgifb_info->refresh_rate =
> - XGIfb_vrate[i].refresh;
> - } else if (((rate - XGIfb_vrate[i - 1].refresh)
> - <= 2) && (XGIfb_vrate[i].idx
> - != 1)) {
> - pr_debug("Adjusting rate from %d down to %d\n",
> - rate,
> - XGIfb_vrate[i - 1].refresh);
> - xgifb_info->rate_idx =
> - XGIfb_vrate[i - 1].idx;
> - xgifb_info->refresh_rate =
> - XGIfb_vrate[i - 1].refresh;
> - }
> - break;
> - } else if ((rate - XGIfb_vrate[i].refresh) <= 2) {
> + xgifb_info->refresh_rate =
> + XGIfb_vrate[i].refresh;
> + } else if ((rate - XGIfb_vrate[i - 1].refresh <= 2)
> + && (XGIfb_vrate[i].idx != 1)) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This bug is there in the original code, and not something that you
introduced but the second part of the if condition is to ensure that
we didn't do an array underflow in the first part of the if statement.
These days that can trigger a kasan warning, I believe.
The conditions should be swapped around to avoid the read before the
start of the array altogether. And, in fact, it should be written like
this to make it easier for static analysis tools:
} else if (i != 0 &&
rate - XGIfb_vrate[i - 1].refresh <= 2) {
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function
2018-03-20 12:03 ` Dan Carpenter
@ 2018-03-20 14:02 ` Pratik Jain
2018-03-21 9:24 ` Dan Carpenter
0 siblings, 1 reply; 10+ messages in thread
From: Pratik Jain @ 2018-03-20 14:02 UTC (permalink / raw)
To: arnaud.patard; +Cc: devel, gregkh, linux-kernel, Pratik Jain
Refactored the function `XGIfb_search_refresh_rate` by removing a level
of `if...else` block nesting. Removed unnecessary parantheses.
Signed-off-by: Pratik Jain <pratik.jain0509@gmail.com>
---
drivers/staging/xgifb/XGI_main_26.c | 59 +++++++++++++++++++------------------
1 file changed, 30 insertions(+), 29 deletions(-)
diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
index 10107de0119a..aee969aab681 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -544,41 +544,42 @@ static u8 XGIfb_search_refresh_rate(struct xgifb_video_info *xgifb_info,
yres = XGIbios_mode[xgifb_info->mode_idx].yres;
xgifb_info->rate_idx = 0;
- while ((XGIfb_vrate[i].idx != 0) && (XGIfb_vrate[i].xres <= xres)) {
- if ((XGIfb_vrate[i].xres == xres) &&
- (XGIfb_vrate[i].yres == yres)) {
- if (XGIfb_vrate[i].refresh == rate) {
+
+ while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres <= xres) {
+ /* Skip values with xres or yres less than specified */
+ if ((XGIfb_vrate[i].yres != yres) ||
+ (XGIfb_vrate[i].xres < xres)) {
+ i++;
+ continue;
+ }
+ if (XGIfb_vrate[i].refresh == rate) {
+ xgifb_info->rate_idx = XGIfb_vrate[i].idx;
+ break;
+ } else if (XGIfb_vrate[i].refresh > rate) {
+ if (XGIfb_vrate[i].refresh - rate <= 3) {
+ pr_debug("Adjusting rate from %d up to %d\n",
+ rate, XGIfb_vrate[i].refresh);
xgifb_info->rate_idx = XGIfb_vrate[i].idx;
- break;
- } else if (XGIfb_vrate[i].refresh > rate) {
- if ((XGIfb_vrate[i].refresh - rate) <= 3) {
- pr_debug("Adjusting rate from %d up to %d\n",
- rate, XGIfb_vrate[i].refresh);
- xgifb_info->rate_idx =
- XGIfb_vrate[i].idx;
- xgifb_info->refresh_rate =
- XGIfb_vrate[i].refresh;
- } else if (((rate - XGIfb_vrate[i - 1].refresh)
- <= 2) && (XGIfb_vrate[i].idx
- != 1)) {
- pr_debug("Adjusting rate from %d down to %d\n",
- rate,
- XGIfb_vrate[i - 1].refresh);
- xgifb_info->rate_idx =
- XGIfb_vrate[i - 1].idx;
- xgifb_info->refresh_rate =
- XGIfb_vrate[i - 1].refresh;
- }
- break;
- } else if ((rate - XGIfb_vrate[i].refresh) <= 2) {
+ xgifb_info->refresh_rate =
+ XGIfb_vrate[i].refresh;
+ } else if ((i > 0) &&
+ (rate - XGIfb_vrate[i - 1].refresh <= 2)) {
pr_debug("Adjusting rate from %d down to %d\n",
- rate, XGIfb_vrate[i].refresh);
- xgifb_info->rate_idx = XGIfb_vrate[i].idx;
- break;
+ rate, XGIfb_vrate[i - 1].refresh);
+ xgifb_info->rate_idx = XGIfb_vrate[i - 1].idx;
+ xgifb_info->refresh_rate =
+ XGIfb_vrate[i - 1].refresh;
}
+ break;
+ } else if (rate - XGIfb_vrate[i].refresh <= 2) {
+ pr_debug("Adjusting rate from %d down to %d\n",
+ rate, XGIfb_vrate[i].refresh);
+ xgifb_info->rate_idx = XGIfb_vrate[i].idx;
+ break;
}
i++;
}
+
if (xgifb_info->rate_idx > 0)
return xgifb_info->rate_idx;
pr_info("Unsupported rate %d for %dx%d\n",
--
2.16.2
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function
2018-03-20 14:02 ` Pratik Jain
@ 2018-03-21 9:24 ` Dan Carpenter
2018-03-21 9:50 ` Pratik Jain
0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2018-03-21 9:24 UTC (permalink / raw)
To: Pratik Jain; +Cc: devel, gregkh, linux-kernel, arnaud.patard
On Tue, Mar 20, 2018 at 07:32:32PM +0530, Pratik Jain wrote:
> Refactored the function `XGIfb_search_refresh_rate` by removing a level
> of `if...else` block nesting. Removed unnecessary parantheses.
>
> Signed-off-by: Pratik Jain <pratik.jain0509@gmail.com>
> ---
> drivers/staging/xgifb/XGI_main_26.c | 59 +++++++++++++++++++------------------
> 1 file changed, 30 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
> index 10107de0119a..aee969aab681 100644
> --- a/drivers/staging/xgifb/XGI_main_26.c
> +++ b/drivers/staging/xgifb/XGI_main_26.c
> @@ -544,41 +544,42 @@ static u8 XGIfb_search_refresh_rate(struct xgifb_video_info *xgifb_info,
> yres = XGIbios_mode[xgifb_info->mode_idx].yres;
>
> xgifb_info->rate_idx = 0;
> - while ((XGIfb_vrate[i].idx != 0) && (XGIfb_vrate[i].xres <= xres)) {
> - if ((XGIfb_vrate[i].xres == xres) &&
> - (XGIfb_vrate[i].yres == yres)) {
> - if (XGIfb_vrate[i].refresh == rate) {
> +
> + while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres <= xres) {
> + /* Skip values with xres or yres less than specified */
> + if ((XGIfb_vrate[i].yres != yres) ||
> + (XGIfb_vrate[i].xres < xres)) {
Both < and != are equivalent, but to me != feels more obvious. With !=
the opposite of that is *always* == so that's obvious just by looking at
it. But with < you have to look at the lines before so you have to
remember more stuff to understand the code.
The other thing is that != xres matches nicely with != yres. Otherwise
I sort of assume from looking at it that != and < are different things.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function
2018-03-21 9:24 ` Dan Carpenter
@ 2018-03-21 9:50 ` Pratik Jain
2018-03-21 10:05 ` Dan Carpenter
0 siblings, 1 reply; 10+ messages in thread
From: Pratik Jain @ 2018-03-21 9:50 UTC (permalink / raw)
To: arnaud.patard; +Cc: devel, gregkh, linux-kernel, Pratik Jain
Refactored the function `XGIfb_search_refresh_rate` by removing a level
of `if...else` block nesting. Removed unnecessary parantheses. Removed
potential bug of array underflow.
Signed-off-by: Pratik Jain <pratik.jain0509@gmail.com>
---
drivers/staging/xgifb/XGI_main_26.c | 59 +++++++++++++++++++------------------
1 file changed, 30 insertions(+), 29 deletions(-)
diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
index 10107de0119a..eca0b50f0df6 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -544,41 +544,42 @@ static u8 XGIfb_search_refresh_rate(struct xgifb_video_info *xgifb_info,
yres = XGIbios_mode[xgifb_info->mode_idx].yres;
xgifb_info->rate_idx = 0;
- while ((XGIfb_vrate[i].idx != 0) && (XGIfb_vrate[i].xres <= xres)) {
- if ((XGIfb_vrate[i].xres == xres) &&
- (XGIfb_vrate[i].yres == yres)) {
- if (XGIfb_vrate[i].refresh == rate) {
+
+ while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres <= xres) {
+ /* Skip values with xres or yres less than specified */
+ if ((XGIfb_vrate[i].yres != yres) ||
+ (XGIfb_vrate[i].xres != xres)) {
+ i++;
+ continue;
+ }
+ if (XGIfb_vrate[i].refresh == rate) {
+ xgifb_info->rate_idx = XGIfb_vrate[i].idx;
+ break;
+ } else if (XGIfb_vrate[i].refresh > rate) {
+ if (XGIfb_vrate[i].refresh - rate <= 3) {
+ pr_debug("Adjusting rate from %d up to %d\n",
+ rate, XGIfb_vrate[i].refresh);
xgifb_info->rate_idx = XGIfb_vrate[i].idx;
- break;
- } else if (XGIfb_vrate[i].refresh > rate) {
- if ((XGIfb_vrate[i].refresh - rate) <= 3) {
- pr_debug("Adjusting rate from %d up to %d\n",
- rate, XGIfb_vrate[i].refresh);
- xgifb_info->rate_idx =
- XGIfb_vrate[i].idx;
- xgifb_info->refresh_rate =
- XGIfb_vrate[i].refresh;
- } else if (((rate - XGIfb_vrate[i - 1].refresh)
- <= 2) && (XGIfb_vrate[i].idx
- != 1)) {
- pr_debug("Adjusting rate from %d down to %d\n",
- rate,
- XGIfb_vrate[i - 1].refresh);
- xgifb_info->rate_idx =
- XGIfb_vrate[i - 1].idx;
- xgifb_info->refresh_rate =
- XGIfb_vrate[i - 1].refresh;
- }
- break;
- } else if ((rate - XGIfb_vrate[i].refresh) <= 2) {
+ xgifb_info->refresh_rate =
+ XGIfb_vrate[i].refresh;
+ } else if ((XGIfb_vrate[i].idx != 1) &&
+ (rate - XGIfb_vrate[i - 1].refresh <= 2)) {
pr_debug("Adjusting rate from %d down to %d\n",
- rate, XGIfb_vrate[i].refresh);
- xgifb_info->rate_idx = XGIfb_vrate[i].idx;
- break;
+ rate, XGIfb_vrate[i - 1].refresh);
+ xgifb_info->rate_idx = XGIfb_vrate[i - 1].idx;
+ xgifb_info->refresh_rate =
+ XGIfb_vrate[i - 1].refresh;
}
+ break;
+ } else if (rate - XGIfb_vrate[i].refresh <= 2) {
+ pr_debug("Adjusting rate from %d down to %d\n",
+ rate, XGIfb_vrate[i].refresh);
+ xgifb_info->rate_idx = XGIfb_vrate[i].idx;
+ break;
}
i++;
}
+
if (xgifb_info->rate_idx > 0)
return xgifb_info->rate_idx;
pr_info("Unsupported rate %d for %dx%d\n",
--
2.16.2
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function
2018-03-21 9:50 ` Pratik Jain
@ 2018-03-21 10:05 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-03-21 10:05 UTC (permalink / raw)
To: Pratik Jain; +Cc: devel, gregkh, linux-kernel, arnaud.patard
That looks nice. Thank you. Sorry for the headaches.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function
@ 2018-03-19 15:56 Pratik Jain
0 siblings, 0 replies; 10+ messages in thread
From: Pratik Jain @ 2018-03-19 15:56 UTC (permalink / raw)
To: arnaud.patard; +Cc: linux-kernel, Pratik Jain
Refactored the function `XGIfb_search_refresh_rate` by removing a level
of `if...else` block nesting. Removed unnecessary parantheses.
Signed-off-by: Pratik Jain <pratik.jain0509@gmail.com>
---
drivers/staging/xgifb/XGI_main_26.c | 63 +++++++++++++++++++------------------
1 file changed, 33 insertions(+), 30 deletions(-)
diff --git a/drivers/staging/xgifb/XGI_main_26.c b/drivers/staging/xgifb/XGI_main_26.c
index 10107de0119a..ef9a726cd35d 100644
--- a/drivers/staging/xgifb/XGI_main_26.c
+++ b/drivers/staging/xgifb/XGI_main_26.c
@@ -544,41 +544,44 @@ static u8 XGIfb_search_refresh_rate(struct xgifb_video_info *xgifb_info,
yres = XGIbios_mode[xgifb_info->mode_idx].yres;
xgifb_info->rate_idx = 0;
- while ((XGIfb_vrate[i].idx != 0) && (XGIfb_vrate[i].xres <= xres)) {
- if ((XGIfb_vrate[i].xres == xres) &&
- (XGIfb_vrate[i].yres == yres)) {
- if (XGIfb_vrate[i].refresh == rate) {
+
+ // Skip values with less xres
+ while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres < xres)
+ ++i;
+
+ while (XGIfb_vrate[i].idx != 0 && XGIfb_vrate[i].xres <= xres) {
+ if (XGIfb_vrate[i].yres != yres) {
+ ++i;
+ continue;
+ }
+ if (XGIfb_vrate[i].refresh == rate) {
+ xgifb_info->rate_idx = XGIfb_vrate[i].idx;
+ break;
+ } else if (XGIfb_vrate[i].refresh > rate) {
+ if (XGIfb_vrate[i].refresh - rate <= 3) {
+ pr_debug("Adjusting rate from %d up to %d\n",
+ rate, XGIfb_vrate[i].refresh);
xgifb_info->rate_idx = XGIfb_vrate[i].idx;
- break;
- } else if (XGIfb_vrate[i].refresh > rate) {
- if ((XGIfb_vrate[i].refresh - rate) <= 3) {
- pr_debug("Adjusting rate from %d up to %d\n",
- rate, XGIfb_vrate[i].refresh);
- xgifb_info->rate_idx =
- XGIfb_vrate[i].idx;
- xgifb_info->refresh_rate =
- XGIfb_vrate[i].refresh;
- } else if (((rate - XGIfb_vrate[i - 1].refresh)
- <= 2) && (XGIfb_vrate[i].idx
- != 1)) {
- pr_debug("Adjusting rate from %d down to %d\n",
- rate,
- XGIfb_vrate[i - 1].refresh);
- xgifb_info->rate_idx =
- XGIfb_vrate[i - 1].idx;
- xgifb_info->refresh_rate =
- XGIfb_vrate[i - 1].refresh;
- }
- break;
- } else if ((rate - XGIfb_vrate[i].refresh) <= 2) {
+ xgifb_info->refresh_rate =
+ XGIfb_vrate[i].refresh;
+ } else if ((rate - XGIfb_vrate[i - 1].refresh <= 2)
+ && (XGIfb_vrate[i].idx != 1)) {
pr_debug("Adjusting rate from %d down to %d\n",
- rate, XGIfb_vrate[i].refresh);
- xgifb_info->rate_idx = XGIfb_vrate[i].idx;
- break;
+ rate, XGIfb_vrate[i - 1].refresh);
+ xgifb_info->rate_idx = XGIfb_vrate[i - 1].idx;
+ xgifb_info->refresh_rate =
+ XGIfb_vrate[i - 1].refresh;
}
+ break;
+ } else if (rate - XGIfb_vrate[i].refresh <= 2) {
+ pr_debug("Adjusting rate from %d down to %d\n",
+ rate, XGIfb_vrate[i].refresh);
+ xgifb_info->rate_idx = XGIfb_vrate[i].idx;
+ break;
}
- i++;
+ ++i;
}
+
if (xgifb_info->rate_idx > 0)
return xgifb_info->rate_idx;
pr_info("Unsupported rate %d for %dx%d\n",
--
2.16.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-03-21 10:05 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-20 8:35 [PATCH] Staging: xgifb: XGI_main_26.c: Refactored the function Pratik Jain
2018-03-20 11:20 ` Dan Carpenter
2018-03-20 11:41 ` Pratik Jain
2018-03-20 11:54 ` Pratik Jain
2018-03-20 12:03 ` Dan Carpenter
2018-03-20 14:02 ` Pratik Jain
2018-03-21 9:24 ` Dan Carpenter
2018-03-21 9:50 ` Pratik Jain
2018-03-21 10:05 ` Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2018-03-19 15:56 Pratik Jain
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).