Re: [PATCH 08/13 v3] ov6650: convert to the control framework.

[Sylwester Nawrocki <snjw23@gmail.com>]

Hi,

On 09/09/2011 08:01 PM, 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@cisco.com>
>>>
>>> Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
>>> [g.liakhovetski@gmx.de: simplified pointer arithmetic]
>>> Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
>>
>> 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@tis.icnet.pl>
>>
>> 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?
> 
> 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.

--
Regards,
Sylwester