On 05/04/2015 09:40 AM, Guennadi Liakhovetski wrote:
> On Mon, 4 May 2015, Hans Verkuil wrote:
>
>> On 05/03/2015 11:02 PM, Guennadi Liakhovetski wrote:
>>> Hi Hans,
>>>
>>> On Sun, 3 May 2015, Hans Verkuil wrote:
>>>
>>>> From: Hans Verkuil <hans.verk...@cisco.com>
>>>>
>>>> The left and top variables were uninitialized, leading to unexpected
>>>> results.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
>>>> ---
>>>> drivers/media/i2c/soc_camera/mt9t112.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/i2c/soc_camera/mt9t112.c
>>>> b/drivers/media/i2c/soc_camera/mt9t112.c
>>>> index de10a76..02190d6 100644
>>>> --- a/drivers/media/i2c/soc_camera/mt9t112.c
>>>> +++ b/drivers/media/i2c/soc_camera/mt9t112.c
>>>> @@ -952,7 +952,8 @@ static int mt9t112_set_fmt(struct v4l2_subdev *sd,
>>>> struct v4l2_mbus_framefmt *mf = &format->format;
>>>> struct i2c_client *client = v4l2_get_subdevdata(sd);
>>>> struct mt9t112_priv *priv = to_mt9t112(client);
>>>> - unsigned int top, left;
>>>> + unsigned int top = priv->frame.top;
>>>> + unsigned int left = priv->frame.left;
>>>
>>> I don't think this is needed. We don't care about left and top in
>>> mt9t112_set_fmt().
>>
>> On further analysis you are correct, it will work with random left/top
>> values. But I think it is 1) very unexpected and 2) bad form to leave it
>> with random values.
>>
>> I prefer to keep this patch, unless you disagree.
>
> Sorry, but I do. Assigning those specific values to left and top makes the
> code even more confusing, it makes it look like that makes any sense,
> whereas it doesn't. If anything we can add a comment there. Or we can pass
> NULL and make sure to catch it somewhere down the line.
>
What about this:
unsigned int top = 0; /* don't care */
unsigned int left = 0; /* don't care */
Regards,
Hans
> Thanks
> Guennadi
>
>> Regards,
>>
>> Hans
>>
>>>
>>> How about my comment about a duplicated call to mt9t112_set_params()? Can
>>> we have it fixed too?
>>>
>>> Thanks
>>> Guennadi
>>>
>>>> int i;
>>>>
>>>> if (format->pad)
>>>> --
>>>> 2.1.4
>>>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
