Re: [PATCH v1 3/3] v4l: Add v4l2 subdev driver for S5K6AAFX sensor

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

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