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 Nawrockisylvester.nawrocki@gmail.com 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