Hi,
On 02/29/2012 10:35 AM, Laurent Pinchart wrote:
>>>> + if (sensor->pixel_array->ctrl_handler.error) {
>>>> + dev_err(&client->dev,
>>>> + "pixel array controls initialization failed (%d)\n",
>>>> + sensor->pixel_array->ctrl_handler.error);
>>>
>>> Shouldn't you call v4l2_ctrl_handler_free() here ?
>>
>> Yes. Fixed.
>>
>>>> + return sensor->pixel_array->ctrl_handler.error;
>>>> + }
>>>> +
>>>> + sensor->pixel_array->sd.ctrl_handler =
>>>> + &sensor->pixel_array->ctrl_handler;
>>>> +
>>>> + v4l2_ctrl_cluster(2, &sensor->hflip);
>>>
>>> Shouldn't you move this before the control handler check ?
>>
>> Why? It can't fail.
>
> I thought it could fail. You could then leave it here, but it would be easier
> from a maintenance point of view to check the error code after completing all
> control-related initialization, as it would avoid introducing a bug if for
> some reason the v4l2_ctrl_cluster() function needs to return an error later.
By calling v4l2_ctrl_cluster() after the control handler check you're sure
sensor->hflip is a pointer to a valid control. In case the HFLIP control
creation fails and you try to cluster that, unpredictable things may happen.
Well, predictable, e.g. BUG_ON() in v4l2_ctrl_cluster(). :-)
So using v4l2_ctrl_cluster() before checking ctrl_handler.error would require
validating the control pointer or maybe something more.
--
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
