On Fri, 9 Sep 2011 at 20:01:14 Guennadi Liakhovetski wrote: > Hi Janusz > > On Fri, 9 Sep 2011, Janusz Krzysztofik wrote: > > > On Thu, 8 Sep 2011 at 10:44:01 Guennadi Liakhovetski wrote: > > > From: Hans Verkuil <hans.verkuil@xxxxxxxxx> > > > > > > Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> > > > [g.liakhovetski@xxxxxx: simplified pointer arithmetic] > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx> > > > > Hi, > > I've successfully tested this one for you, to the extent possible using > > mplayer, i.e., only saturation, hue and brightness controls, and an > > overall functionality. > > Thanks for testing and reviewing! > > > Modifications to other (not runtime tested) controls look good to me, > > except for one copy-paste mistake, see below. With that erratic REG_BLUE > > corrected: > > > > Acked-by: Janusz Krzysztofik <jkrzyszt@xxxxxxxxxxxx> > > > > There are also a few minor comments for you to consider. > > Well, some of them are not so minor, I would say;-) But I personally would > be happy to have this just as an incremental patch. Would you like to > prepare one or should I do it? OK, I can do it. Do you want them all (except the one below) go into the incremental patch, or would you rather fix that REG_RED bug still before applying? Thanks, Janusz > > I basically agree with all your comments apart from maybe > > [snip] > > > > @@ -1176,9 +1021,11 @@ static int ov6650_probe(struct i2c_client *client, > > > priv->colorspace = V4L2_COLORSPACE_JPEG; > > > > > > ret = ov6650_video_probe(icd, client); > > > + if (!ret) > > > + ret = v4l2_ctrl_handler_setup(&priv->hdl); > > > > Are you sure the probe function should fail if v4l2_ctrl_handler_setup() > > fails? Its usage is documented as optional. > > Not sure what the standard really meant, but it looks like this is done in > all patches in this series. So, we'd have to change this everywhere. Most > other drivers indeed do not care. > > Thanks > Guennadi > --- > Guennadi Liakhovetski, Ph.D. > Freelance Open-Source Software Developer > http://www.open-technology.de/ > -- > 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 > -- 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
