Hello,
Emil Velikov wrote:
> On 31 August 2015 at 14:18, Inki Dae <inki.dae@xxxxxxxxxxx> wrote:
>> 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@xxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>> 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;
>> }
>>
> Listing every value might seem like an overkill, indeed. Yet I think
> it's the more robust approach.
that's my thinking as well. The overhead shouldn't be too high and the
compiler probably optimizes this anyway.
> By adding _MAX to the API we effectively lock it down. That can be
> avoided, plus the compiler will warn us when new values are added to
> the enum. If someone starts getting smart (due to the _MAX) and adds
> G2D_SELECT_MODE_FOO = -1, the above check will fail :(
>
>> And I think it'd be better to change return type of this function to int,
>>
> Good idea.
What would be the benefit of this? We're just returning only 0 and 1
anyway. My first reaction was to use a bool here.
>
> Cheers,
> Emil
>
With best wishes,
Tobias
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
