On Mon, 4 May 2015, Hans Verkuil wrote:
> 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 <[email protected]>
> >>>>
> >>>> The left and top variables were uninitialized, leading to unexpected
> >>>> results.
> >>>>
> >>>> Signed-off-by: Hans Verkuil <[email protected]>
> >>>> ---
> >>>> 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 */
This would be my third preference. My first preference is just a comment.
My second preference is adding
if (!start)
return;
in the middle of soc_camera_limit_side() and using NULL in
mt9t112_set_fmt(). I really dislike meaningless operations, also when they
fix compiler warnings, but here even the compiler is happy:)
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
