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 <hans.verkuil@xxxxxxxxx> > >>>> > >>>> The left and top variables were uninitialized, leading to unexpected > >>>> results. > >>>> > >>>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> > >>>> --- > >>>> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
