Re: [PATCH v8 05/10] media: venus: adding core part and helper functions

From: Stanimir Varbanov <stanimir.varbanov@linaro.org>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Sakari Ailus <sakari.ailus@iki.fi>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	Andy Gross <andy.gross@linaro.org>,
	Stephen Boyd <sboyd@codeaurora.org>,
	Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v8 05/10] media: venus: adding core part and helper functions
Date: Fri, 5 May 2017 16:29:05 +0300	[thread overview]
Message-ID: <7fbca77f-b783-efcb-a39a-4d42ed8db310@linaro.org> (raw)
In-Reply-To: <20170502185241.GX15143@minitux>

Hi Bjorn

On 05/02/2017 09:52 PM, Bjorn Andersson wrote:
> On Tue 02 May 01:52 PDT 2017, Stanimir Varbanov wrote:
> 
>> Hei Sakari,
>>
>> On 04/30/2017 01:21 AM, Sakari Ailus wrote:
>>> Hi, Stan!!
>>>
>>> On Fri, Apr 28, 2017 at 12:13:52PM +0300, Stanimir Varbanov wrote:
>>> ...
>>>> +int helper_get_bufreq(struct venus_inst *inst, u32 type,
>>>> +		      struct hfi_buffer_requirements *req)
>>>> +{
>>>> +	u32 ptype = HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS;
>>>> +	union hfi_get_property hprop;
>>>> +	int ret, i;
>>>
>>> unsigned int i ? It's an array index...
>>
>> Thanks for pointing that out, I have to revisit all similar places as
>> well ...
>>
> 
> It's perfectly fine to index an array with an int and you are comparing
> the index with a integer constant in the loop - so don't clutter the
> code unnecessarily.

I personally prefer unsigned for iterator variable type (because
unsigned type has defined behavior on overflow), but having the fact
that I'm comparing with int I will keep it int.

Also it seems that -Wsign-compare is not enabled by default in kernel,
no? So I have modified my Makefile and catch few occurrences of warnings
about signed with unsigned compare and fixed them.

-- 
regards,
Stan