[PATCHi v3] media: staging/intel-ipu3: css: simplify expression

[PATCHi v3] media: staging/intel-ipu3: css: simplify expression

[Deepak R Varma]

An array index computed inside square brackets complicates the code
and also extends the line beyond 80 character. Add new variable to
compute array index separately and use it as an index during assignment.

Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
---
Changes since v2:
  - Added feedback from Julia
        1. Rephrase patch description to make it concise and simpler.
Changes since v1:
  - Added feedback from Helen
        1. Updated variable type to "unsigned int" from earlier "int"
        2. Implemented the change in another area in same scope
        3. Left newly added variable uninitialised. 

 drivers/staging/media/ipu3/ipu3-css-params.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/ipu3/ipu3-css-params.c b/drivers/staging/media/ipu3/ipu3-css-params.c
index 4533dacad4be..5622319b69ff 100644
--- a/drivers/staging/media/ipu3/ipu3-css-params.c
+++ b/drivers/staging/media/ipu3/ipu3-css-params.c
@@ -49,13 +49,13 @@ imgu_css_scaler_setup_lut(unsigned int taps, unsigned int input_width,
 	int tap, phase, phase_sum_left, phase_sum_right;
 	int exponent = imgu_css_scaler_get_exp(output_width, input_width);
 	int mantissa = (1 << exponent) * output_width;
-	unsigned int phase_step;
+	unsigned int phase_step, phase_taps;
 
 	if (input_width == output_width) {
 		for (phase = 0; phase < IMGU_SCALER_PHASES; phase++) {
+			phase_taps = phase * IMGU_SCALER_FILTER_TAPS;
 			for (tap = 0; tap < taps; tap++) {
-				coeff_lut[phase * IMGU_SCALER_FILTER_TAPS + tap]
-					= 0;
+				coeff_lut[phase_taps + tap] = 0;
 			}
 		}
 
@@ -82,8 +82,8 @@ imgu_css_scaler_setup_lut(unsigned int taps, unsigned int input_width,
 			coeff += 1 << (IMGU_SCALER_COEFF_BITS - 1);
 			coeff >>= IMGU_SCALER_COEFF_BITS;
 
-			coeff_lut[phase * IMGU_SCALER_FILTER_TAPS + tap] =
-				coeff;
+			phase_taps = phase * IMGU_SCALER_FILTER_TAPS + tap;
+			coeff_lut[phase_taps] = coeff;
 		}
 	}
 
-- 
2.17.1

Re: [Outreachy kernel] [PATCHi v3] media: staging/intel-ipu3: css: simplify expression

[Stefano Brivio]

Subject has an extra 'i' after PATCH (this might actually confuse some
systems for processing patches). Two comments:

On Sat, 14 Mar 2020 03:58:15 +0530
Deepak R Varma <mh12gx2825@gmail.com> wrote:

> An array index computed inside square brackets complicates the code
> and also extends the line beyond 80 character. Add new variable to
> compute array index separately and use it as an index during assignment.
> 
> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> ---
> Changes since v2:
>   - Added feedback from Julia
>         1. Rephrase patch description to make it concise and simpler.
> Changes since v1:
>   - Added feedback from Helen
>         1. Updated variable type to "unsigned int" from earlier "int"
>         2. Implemented the change in another area in same scope
>         3. Left newly added variable uninitialised. 
> 
>  drivers/staging/media/ipu3/ipu3-css-params.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/ipu3/ipu3-css-params.c b/drivers/staging/media/ipu3/ipu3-css-params.c
> index 4533dacad4be..5622319b69ff 100644
> --- a/drivers/staging/media/ipu3/ipu3-css-params.c
> +++ b/drivers/staging/media/ipu3/ipu3-css-params.c
> @@ -49,13 +49,13 @@ imgu_css_scaler_setup_lut(unsigned int taps, unsigned int input_width,
>  	int tap, phase, phase_sum_left, phase_sum_right;
>  	int exponent = imgu_css_scaler_get_exp(output_width, input_width);
>  	int mantissa = (1 << exponent) * output_width;
> -	unsigned int phase_step;
> +	unsigned int phase_step, phase_taps;
>  
>  	if (input_width == output_width) {
>  		for (phase = 0; phase < IMGU_SCALER_PHASES; phase++) {
> +			phase_taps = phase * IMGU_SCALER_FILTER_TAPS;
>  			for (tap = 0; tap < taps; tap++) {
> -				coeff_lut[phase * IMGU_SCALER_FILTER_TAPS + tap]
> -					= 0;
> +				coeff_lut[phase_taps + tap] = 0;
>  			}

I'm not sure if checkpatch reports this (I haven't tried), but now you
can remove the curly brackets.

>  		}
>  
> @@ -82,8 +82,8 @@ imgu_css_scaler_setup_lut(unsigned int taps, unsigned int input_width,
>  			coeff += 1 << (IMGU_SCALER_COEFF_BITS - 1);
>  			coeff >>= IMGU_SCALER_COEFF_BITS;
>  
> -			coeff_lut[phase * IMGU_SCALER_FILTER_TAPS + tap] =
> -				coeff;
> +			phase_taps = phase * IMGU_SCALER_FILTER_TAPS + tap;
> +			coeff_lut[phase_taps] = coeff;

For consistency with the input_width == output_width case, I would
instead keep phase_taps as phase * IMGU_SCALER_FILTER_TAPS (otherwise
the same variable has two rather different meanings), and, here, assign
it in the outer loop.

-- 
Stefano