Hi Sangwook,
On 08/03/2012 05:05 PM, Sangwook Lee wrote:
>>> +/* configure 30 fps */
>>> +static const struct regval_list s5k4ecgx_fps_30[] = {
>>
>> It really depends on sensor master clock frequency (as specified
>> in FIMC driver platform data) and PLL setting what the resulting
>> frame rate will be.
>>
>>> + { 0x700002b4, 0x0052 },
>>
>> Looks surprising! Are we really just disabling horizontal/vertical
>> image mirror here ?
>
> I believe, this setting values are used still in Galaxy Nexus.
> It might be some reasons to set this values in the product, but I am not
> sure of this.
My point was that some entries in this table allegedly are setting image
mirroring, even though the array name suggests it should be only setting
frame rate to 30 fps. This is just bad practice. If you would have added
HFLIP/VFLIP controls, the register values would have been trashed every
time frame rate is set. Someone would have eventually have had to get rid
of this s5k4ecgx_fps_30 array, in order to add new features.
> [snip]
>>> + { 0xffffffff, 0x0000 },
>>> +};
>>
>> You already use a sequence of i2c writes in s5k4ecgx_s_ctrl() function
>> for V4L2_CID_SHARPNESS control. So why not just create e.g.
>> s5k4ecgx_set_saturation() and send this array to /dev/null ?
>> Also, invoking v4l2_ctrl_handler_setup() will cause a call to s5k4ecgx_s_ctrl()
>> with default sharpness value (as specified during the control's creation).
>>
>> So I would say this array is redundant in two ways... :)
>
> Thanks, let me change this.
Thanks, please at least remove those single entry arrays, with the
resolution arrays gone as well and the biggest array converted to
firmware blob I don't see a reason why this driver couldn't be accepted
upstream.
--
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
