Hi Sangwook,
On 08/03/2012 04:24 PM, Sangwook Lee wrote:
> Hi Sylwester
>
> Thank you for the review.
>
> On 2 August 2012 21:11, Sylwester Nawrocki<[email protected]>
> wrote:
>> On 08/02/2012 03:42 PM, Sangwook Lee wrote:
>>> The following 2 patches add driver for S5K4ECGX sensor with embedded ISP
>>> SoC,
>>> and minor v4l2 control API enhancement. S5K4ECGX is 5M CMOS Image sensor
>>> from Samsung
>>>
>>> Changes since v2:
>>> - added GPIO (reset/stby) and regulators
>>> - updated I2C read/write, based on s5k6aa datasheet
>>> - fixed set_fmt errors
>>> - reduced register tables a bit
>>> - removed vmalloc
>>
>> It looks like a great improvement, well done! Thanks!
>>
>> In the S5K4CAGX sensor datasheet, that can be found on the internet,
>> there is 0x0000...0x002E registers description, which look very much
>> same as in S5K6AAFX case and likely is also valid for S5K4CAGX.
>
>
> [snip]
>
>>
>>
>> What do you think about converting s5k4ecgx_img_regs arrays (it has
>> over 2700 entries) to a firmware file and adding some simple parser
>> to the driver ? Then we would have the problem solved in most part.
>>
>
> Thanks, fair enough. let me try this.
All right, thanks.
>> Regarding various preview resolution set up, the difference in all
>> those s5k4ecgx_*_preview[] arrays is rather small, only register
>> values differ, e.g. for 640x480 and 720x480 there is only 8 different
>> entries:
>>
>
> Ok, let me reduce table size again.
I don't think it's worth the effort to work around those tables.
They may just be removed entirely. I'll see if I can find time to
prepare a function replacing them. All required information seems
to be available in the datasheet.
>> $ diff -a s5k4ec_640.txt s5k4ec_720.txt
>> 1c1
>> < static const struct regval_list s5k4ecgx_640_preview[] = {
>> ---
>>> static const struct regval_list s5k4ecgx_720_preview[] = {
>> 3c3
>> < { 0x70000252, 0x0780 },
>> ---
>>> { 0x70000252, 0x06a8 },
>> 5c5
>
> [snip]
>
>> < { 0x70000256, 0x000c },
>>> { 0x700002a6, 0x02d
>
> [snip]
>>
>> Could you please try to implement a function that replaces those tables,
>> based s5k6aa_set_prev_config() and s5k6aa_set_output_framefmt() ?
>>
> I was thinking about this, but this seems to be is a bit time-consuming
> because
> I have to do this just due to lack of s5k4ecgx hardware information.
> let me try it later once
> this patch is accepted.
Yes, I know it's not trivial and requires some work... Let me try
to cook up something myself, as I have already some experience with
S5K6AAFX. Those "firmware" arrays are evil, and no good driver shall
rely on them.. :-)
And we have plenty time now, until v3.7 merge window.
--
Regards,
Sylwester
_______________________________________________
linaro-dev mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/linaro-dev
