2017-03-30 06:29:32

by Daeseok Youn

Subject: [PATCH 2/2] staging: atomisp: use local variable to reduce the number of reference

Define new local variable to reduce the number of reference.
The new local variable is added to save the addess of dfs
and used in atomisp_freq_scaling() function.

Signed-off-by: Daeseok Youn <[email protected]>
---
.../media/atomisp/pci/atomisp2/atomisp_cmd.c | 37 ++++++++++++----------
1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index eebfccd..d76a95c 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -251,6 +251,7 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
{
/* FIXME! Only use subdev[0] status yet */
struct atomisp_sub_device *asd = &isp->asd[0];
+ const struct atomisp_dfs_config *dfs;
unsigned int new_freq;
struct atomisp_freq_scaling_rule curr_rules;
int i, ret;
@@ -268,20 +269,22 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
ATOMISP_USE_YUVPP(asd))
isp->dfs = &dfs_config_cht_soc;

- if (isp->dfs->lowest_freq == 0 || isp->dfs->max_freq_at_vmin == 0 ||
- isp->dfs->highest_freq == 0 || isp->dfs->dfs_table_size == 0 ||
- !isp->dfs->dfs_table) {
+ dfs = isp->dfs;
+
+ if (dfs->lowest_freq == 0 || dfs->max_freq_at_vmin == 0 ||
+ dfs->highest_freq == 0 || dfs->dfs_table_size == 0 ||
+ !dfs->dfs_table) {
dev_err(isp->dev, "DFS configuration is invalid.\n");
return -EINVAL;
}

if (mode == ATOMISP_DFS_MODE_LOW) {
- new_freq = isp->dfs->lowest_freq;
+ new_freq = dfs->lowest_freq;
goto done;
}

if (mode == ATOMISP_DFS_MODE_MAX) {
- new_freq = isp->dfs->highest_freq;
+ new_freq = dfs->highest_freq;
goto done;
}

@@ -307,26 +310,26 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
}

/* search for the target frequency by looping freq rules*/
- for (i = 0; i < isp->dfs->dfs_table_size; i++) {
- if (curr_rules.width != isp->dfs->dfs_table[i].width &&
- isp->dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
+ for (i = 0; i < dfs->dfs_table_size; i++) {
+ if (curr_rules.width != dfs->dfs_table[i].width &&
+ dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
continue;
- if (curr_rules.height != isp->dfs->dfs_table[i].height &&
- isp->dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
+ if (curr_rules.height != dfs->dfs_table[i].height &&
+ dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
continue;
- if (curr_rules.fps != isp->dfs->dfs_table[i].fps &&
- isp->dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
+ if (curr_rules.fps != dfs->dfs_table[i].fps &&
+ dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
continue;
- if (curr_rules.run_mode != isp->dfs->dfs_table[i].run_mode &&
- isp->dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
+ if (curr_rules.run_mode != dfs->dfs_table[i].run_mode &&
+ dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
continue;
break;
}

- if (i == isp->dfs->dfs_table_size)
- new_freq = isp->dfs->max_freq_at_vmin;
+ if (i == dfs->dfs_table_size)
+ new_freq = dfs->max_freq_at_vmin;
else
- new_freq = isp->dfs->dfs_table[i].isp_freq;
+ new_freq = dfs->dfs_table[i].isp_freq;

done:
dev_dbg(isp->dev, "DFS target frequency=%d.\n", new_freq);
--
1.9.1

2017-03-30 07:19:29

by walter harms

Subject: Re: [PATCH 2/2] staging: atomisp: use local variable to reduce the number of reference

Am 30.03.2017 08:25, schrieb Daeseok Youn:
> Define new local variable to reduce the number of reference.
> The new local variable is added to save the addess of dfs
> and used in atomisp_freq_scaling() function.
>
> Signed-off-by: Daeseok Youn <[email protected]>
> ---
> .../media/atomisp/pci/atomisp2/atomisp_cmd.c | 37 ++++++++++++----------
> 1 file changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> index eebfccd..d76a95c 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> @@ -251,6 +251,7 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
> {
> /* FIXME! Only use subdev[0] status yet */
> struct atomisp_sub_device *asd = &isp->asd[0];
> + const struct atomisp_dfs_config *dfs;
> unsigned int new_freq;
> struct atomisp_freq_scaling_rule curr_rules;
> int i, ret;
> @@ -268,20 +269,22 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
> ATOMISP_USE_YUVPP(asd))
> isp->dfs = &dfs_config_cht_soc;
>
> - if (isp->dfs->lowest_freq == 0 || isp->dfs->max_freq_at_vmin == 0 ||
> - isp->dfs->highest_freq == 0 || isp->dfs->dfs_table_size == 0 ||
> - !isp->dfs->dfs_table) {
> + dfs = isp->dfs;
> +
> + if (dfs->lowest_freq == 0 || dfs->max_freq_at_vmin == 0 ||
> + dfs->highest_freq == 0 || dfs->dfs_table_size == 0 ||
> + !dfs->dfs_table) {
> dev_err(isp->dev, "DFS configuration is invalid.\n");
> return -EINVAL;
> }
>
> if (mode == ATOMISP_DFS_MODE_LOW) {
> - new_freq = isp->dfs->lowest_freq;
> + new_freq = dfs->lowest_freq;
> goto done;
> }
>
> if (mode == ATOMISP_DFS_MODE_MAX) {
> - new_freq = isp->dfs->highest_freq;
> + new_freq = dfs->highest_freq;
> goto done;
> }
>
> @@ -307,26 +310,26 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
> }
>
> /* search for the target frequency by looping freq rules*/
> - for (i = 0; i < isp->dfs->dfs_table_size; i++) {
> - if (curr_rules.width != isp->dfs->dfs_table[i].width &&
> - isp->dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
> + for (i = 0; i < dfs->dfs_table_size; i++) {
> + if (curr_rules.width != dfs->dfs_table[i].width &&
> + dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
> continue;
> - if (curr_rules.height != isp->dfs->dfs_table[i].height &&
> - isp->dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
> + if (curr_rules.height != dfs->dfs_table[i].height &&
> + dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
> continue;
> - if (curr_rules.fps != isp->dfs->dfs_table[i].fps &&
> - isp->dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
> + if (curr_rules.fps != dfs->dfs_table[i].fps &&
> + dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
> continue;
> - if (curr_rules.run_mode != isp->dfs->dfs_table[i].run_mode &&
> - isp->dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
> + if (curr_rules.run_mode != dfs->dfs_table[i].run_mode &&
> + dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
> continue;
> break;
> }

>
> - if (i == isp->dfs->dfs_table_size)
> - new_freq = isp->dfs->max_freq_at_vmin;
> + if (i == dfs->dfs_table_size)
> + new_freq = dfs->max_freq_at_vmin;
> else
> - new_freq = isp->dfs->dfs_table[i].isp_freq;
> + new_freq = dfs->dfs_table[i].isp_freq;
>

you can eliminate the last block by setting

new_freq = dfs->max_freq_at_vmin;

for(i=0;....) {
....
new_freq = dfs->dfs_table[i].isp_freq;
break;
}

unfortunately i have no good idea how to make the loop more readable.


re,
wh


> done:
> dev_dbg(isp->dev, "DFS target frequency=%d.\n", new_freq);

2017-03-30 10:26:58

by Daeseok Youn

Subject: Re: [PATCH 2/2] staging: atomisp: use local variable to reduce the number of reference

2017-03-30 16:19 GMT+09:00 walter harms <[email protected]>:
>
>
> Am 30.03.2017 08:25, schrieb Daeseok Youn:
>> Define new local variable to reduce the number of reference.
>> The new local variable is added to save the addess of dfs
>> and used in atomisp_freq_scaling() function.
>>
>> Signed-off-by: Daeseok Youn <[email protected]>
>> ---
>> .../media/atomisp/pci/atomisp2/atomisp_cmd.c | 37 ++++++++++++----------
>> 1 file changed, 20 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>> index eebfccd..d76a95c 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>> @@ -251,6 +251,7 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
>> {
>> /* FIXME! Only use subdev[0] status yet */
>> struct atomisp_sub_device *asd = &isp->asd[0];
>> + const struct atomisp_dfs_config *dfs;
>> unsigned int new_freq;
>> struct atomisp_freq_scaling_rule curr_rules;
>> int i, ret;
>> @@ -268,20 +269,22 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
>> ATOMISP_USE_YUVPP(asd))
>> isp->dfs = &dfs_config_cht_soc;
>>
>> - if (isp->dfs->lowest_freq == 0 || isp->dfs->max_freq_at_vmin == 0 ||
>> - isp->dfs->highest_freq == 0 || isp->dfs->dfs_table_size == 0 ||
>> - !isp->dfs->dfs_table) {
>> + dfs = isp->dfs;
>> +
>> + if (dfs->lowest_freq == 0 || dfs->max_freq_at_vmin == 0 ||
>> + dfs->highest_freq == 0 || dfs->dfs_table_size == 0 ||
>> + !dfs->dfs_table) {
>> dev_err(isp->dev, "DFS configuration is invalid.\n");
>> return -EINVAL;
>> }
>>
>> if (mode == ATOMISP_DFS_MODE_LOW) {
>> - new_freq = isp->dfs->lowest_freq;
>> + new_freq = dfs->lowest_freq;
>> goto done;
>> }
>>
>> if (mode == ATOMISP_DFS_MODE_MAX) {
>> - new_freq = isp->dfs->highest_freq;
>> + new_freq = dfs->highest_freq;
>> goto done;
>> }
>>
>> @@ -307,26 +310,26 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
>> }
>>
>> /* search for the target frequency by looping freq rules*/
>> - for (i = 0; i < isp->dfs->dfs_table_size; i++) {
>> - if (curr_rules.width != isp->dfs->dfs_table[i].width &&
>> - isp->dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
>> + for (i = 0; i < dfs->dfs_table_size; i++) {
>> + if (curr_rules.width != dfs->dfs_table[i].width &&
>> + dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
>> continue;
>> - if (curr_rules.height != isp->dfs->dfs_table[i].height &&
>> - isp->dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
>> + if (curr_rules.height != dfs->dfs_table[i].height &&
>> + dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
>> continue;
>> - if (curr_rules.fps != isp->dfs->dfs_table[i].fps &&
>> - isp->dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
>> + if (curr_rules.fps != dfs->dfs_table[i].fps &&
>> + dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
>> continue;
>> - if (curr_rules.run_mode != isp->dfs->dfs_table[i].run_mode &&
>> - isp->dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
>> + if (curr_rules.run_mode != dfs->dfs_table[i].run_mode &&
>> + dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
>> continue;
>> break;
>> }
>
>>
>> - if (i == isp->dfs->dfs_table_size)
>> - new_freq = isp->dfs->max_freq_at_vmin;
>> + if (i == dfs->dfs_table_size)
>> + new_freq = dfs->max_freq_at_vmin;
>> else
>> - new_freq = isp->dfs->dfs_table[i].isp_freq;
>> + new_freq = dfs->dfs_table[i].isp_freq;
>>
>
> you can eliminate the last block by setting
>
> new_freq = dfs->max_freq_at_vmin;
>
> for(i=0;....) {
> ....
> new_freq = dfs->dfs_table[i].isp_freq;
> break;
> }
Yes, it could be. I will make another patch to improve it as your comment.
>
> unfortunately i have no good idea how to make the loop more readable.
I am not sure whether the for-loop is possible to improve for
readability or not. :-)

Thanks for comment.
Regards,
Daeseok.
>
>
> re,
> wh
>
>
>> done:
>> dev_dbg(isp->dev, "DFS target frequency=%d.\n", new_freq);