On Fri, 9 Sep 2011, Janusz Krzysztofik wrote: > 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? Just put them all in one. Thanks Guennadi > > 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 > > > --- 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
