Re: [PATCH 07/14] exynos/fimg2d: add g2d_validate_xyz() functions

[Inki Dae <inki.dae@samsung.com>]

On 2015년 08월 24일 23:14, Tobias Jakobi wrote:
> The G2D headers define a number of modes through enums
> (like e.g. color, select, repeat, etc.).
> 
> This introduces g2d_validate_select_mode() and
> g2d_validate_blending_op() which validate a
> select mode or blending operation respectively.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  exynos/exynos_fimg2d.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
> index 2e04f4a..781aff5 100644
> --- a/exynos/exynos_fimg2d.c
> +++ b/exynos/exynos_fimg2d.c
> @@ -119,6 +119,50 @@ static unsigned int g2d_check_space(const struct g2d_context *ctx,
>  }
>  
>  /*
> + * g2d_validate_select_mode - validate select mode.
> + *
> + * @mode: the mode to validate
> + */
> +static unsigned int g2d_validate_select_mode(
> +	enum e_g2d_select_mode mode)
> +{
> +	switch (mode) {
> +	case G2D_SELECT_MODE_NORMAL:
> +	case G2D_SELECT_MODE_FGCOLOR:
> +	case G2D_SELECT_MODE_BGCOLOR:
> +		return 0;
> +	}

It's strange use a bit. Just check the range like below,

First, please add G2D_SELECT_MODE_MAX which has 3 << 0, and

if (G2D_SELECT_MODE_MAX < mode) {
	fprintf(strerr, "invalid command(0x%x)\n", mode);
	return -EINVAL;
}

And I think it'd be better to change return type of this function to int,

> +
> +	return 1;

so return 0

> +}
> +
> +/*
> + * g2d_validate_blending_op - validate blending operation.
> + *
> + * @operation: the operation to validate
> + */
> +static unsigned int g2d_validate_blending_op(
> +	enum e_g2d_op operation)
> +{
> +	switch (operation) {
> +	case G2D_OP_CLEAR:
> +	case G2D_OP_SRC:
> +	case G2D_OP_DST:
> +	case G2D_OP_OVER:
> +	case G2D_OP_INTERPOLATE:
> +	case G2D_OP_DISJOINT_CLEAR:
> +	case G2D_OP_DISJOINT_SRC:
> +	case G2D_OP_DISJOINT_DST:
> +	case G2D_OP_CONJOINT_CLEAR:
> +	case G2D_OP_CONJOINT_SRC:
> +	case G2D_OP_CONJOINT_DST:
> +		return 0;

Ditto, You could modify it like above.

Thanks,
Inki Dae

> +	}
> +
> +	return 1;
> +}
> +
> +/*
>   * g2d_add_cmd - set given command and value to user side command buffer.
>   *
>   * @ctx: a pointer to g2d_context structure.
>