On 09/09/2011 10:58 PM, Janusz Krzysztofik wrote: > On Fri, 9 Sep 2011 at 20:23:38 Sylwester Nawrocki wrote: >> 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? I guess, on individual cases - yes, but I wouldn't take it as a general rule. If the device fails from the beginning it will probably not be really usable thereafter. -- Regards, Sylwester -- 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
