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<sylvester.nawro...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html