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

[Stefano Brivio <sbrivio@redhat.com>]

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