Hi Sakari,
On 09/21/2011 12:10 AM, Sakari Ailus wrote:
> On Tue, Sep 20, 2011 at 01:58:59PM +0200, Sylwester Nawrocki wrote:
>> This driver exposes preview mode operation of the S5K6AAFX sensor with
>> embedded SoC ISP. It uses one of the five user predefined configuration
>> register sets. There is yet no support for capture (snapshot) operation.
>> Following controls are supported:
>> manual/auto exposure and gain, power line frequency (anti-flicker),
>> saturation, sharpness, brightness, contrast, white balance temperature,
>> color effects. horizontal/vertical image flip, frame interval.
>
> Thanks for the patch, Sylwester!
>
> [clip]
>> + v4l2_ctrl_new_std_menu(hdl, ops, V4L2_CID_POWER_LINE_FREQUENCY,
>> + V4L2_CID_POWER_LINE_FREQUENCY_AUTO, 0,
>> + V4L2_CID_POWER_LINE_FREQUENCY_50HZ);
>> +
>> + v4l2_ctrl_new_std_menu(hdl, ops, V4L2_CID_COLORFX,
>> + V4L2_COLORFX_SKETCH, 0x3D0, V4L2_COLORFX_NONE);
>
> New items may be added to standard menus so you should mask out also
> undefined bits. Say, ~0x42f (hope I got that right).
Sure, that's an important detail. ~0x42 look like the right value. Thanks for
pointing this out.
>
> Youd also don't need to check for invalid menu ids; the control framework
> does this for you.
Right, good catch. I'll modify accordingly.
>
>> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_WHITE_BALANCE_TEMPERATURE,
>> + 0, 256, 1, 0);
>> +
>> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SATURATION, -127, 127, 1, 0);
>> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SHARPNESS, -127, 127, 1, 0);
>> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS, -127, 127, 1, 0);
>> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST, -127, 127, 1, 0);
>> +
>> + s5k6aa->sd.ctrl_handler = hdl;
>
> Shoudln't this assignment be done after checking for the error?
Indeed, seems much more appropriate.
>
>> + if (hdl->error) {
>> + ret = hdl->error;
>> + v4l2_ctrl_handler_free(hdl);
>> + }
>> + return ret;
--
Thanks!
Sylwester
--
Sylwester Nawrocki
Samsung Poland R&D Center
--
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
