On Fri, 9 Sep 2011, Janusz Krzysztofik wrote: > On Fri, 9 Sep 2011 at 20:23:38 Sylwester Nawrocki wrote: > > Hi, > > > > On 09/09/2011 08:01 PM, Guennadi Liakhovetski wrote: > > [snip] > > > > 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. > > > > The usage of v4l2_ctrl_handler_setup() is optional, but if this function > > is not used, then AFAIU the driver writer needs to ensure the control's > > values after the device is initialized are exactly as those specified during > > the control creation. Of course v4l2_ctrl_handler_setup() failure might > > mean s_ctrl op failed, which might be caused by some H/W access errors. > > So IMHO it is always a good idea to check the return value if we know > > the batch controls setup shouldn't fail. > > I'm not for ignoring that return value, only wondering if the i2c_driver > .probe handler should really fail in such cases, effectivelly preventing > the device from being accessible at all. > > Perhaps a warning message would be sufficient? Well, restoring the state is normally something pretty basic like writing a couple of registers via i2c. If that didn't work out, there's little chance you'll be able to use the device anyway. So, I'd feel pretty safe with erroring out. If we ever encounter a case, where this can fail in a non-fatal way, we can always relax it. 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
