[PATCH v2 03/14] media: staging: rkisp1: params: use the new effect value in cproc config

[Helen Koike]

On 8/15/20 7:37 AM, Dafna Hirschfeld wrote:
> The cproc (color processing) configuration needs to know if
> an image effect is configured. The code uses the image effect
> in 'cur_params' which is the first params buffer queued in the stream.
> This is the wrong place to read the value. The value should be
> taken from the current params buffer.

This is a bit confusing, since params->cur_params is not the current
parameter as the name suggest, but it is only the first one.

I was thinking we could rename this variable to
`first_params`, and this descriptions would be less confusing,
but I saw you removed it on patch 07/14, so never mind.

It would be easier to read if this patch was just before patch
07/14.

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld at collabora.com>

Acked-by: Helen Koike <helen.koike at collabora.com>

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 8d881f1df1e6..eb77b4ed8655 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -560,7 +560,7 @@ static void rkisp1_cproc_config(struct rkisp1_params *params,
>  				const struct rkisp1_cif_isp_cproc_config *arg)
>  {
>  	struct rkisp1_cif_isp_isp_other_cfg *cur_other_cfg =

Side note: I was also thinking that the prefix "cur_other" is very confusing.
What "other" mean here?

Regards,
Helen

> -						&params->cur_params.others;
> +		container_of(arg, struct rkisp1_cif_isp_isp_other_cfg, cproc_config);
>  	struct rkisp1_cif_isp_ie_config *cur_ie_config =
>  						&cur_other_cfg->ie_config;
>  	u32 effect = cur_ie_config->effect;
>