Hi  Sylwester

Thank you for the review.

On 2 August 2012 21:11, Sylwester Nawrocki <sylvester.nawro...@gmail.com> wrote:
>
> Hi Sangwook,
>
> 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.


>
> 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.

>
> $ 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.

Thanks
Sangwook

> 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

Reply via email to