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 */ 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
