Re: [PATCH v2 2/2] v4l: Add v4l2 subdev driver for S5K4ECGX sensor

[Sangwook Lee <sangwook.lee@linaro.org>]

Hi Sylwester

Thank you for the great review!

On 19 July 2012 22:40, Sylwester Nawrocki <sylvester.nawrocki@gmail.com> wrote:
>
> Hi Sangwook,
>
> A few review comments for you below...
>
> On 07/19/2012 02:14 PM, Sangwook Lee wrote:
> > This dirver implements preview mode of the S5K4ECGX sensor.
>
> dirver -> driver

OK, I will fix this.

> > capture (snapshot) operation, face detection are missing now.
> >
> > Following controls are supported:
> > contrast/saturation/birghtness/sharpness
>
> birghtness -> brightness
>

ditto

> > Signed-off-by: Sangwook Lee<sangwook.lee@linaro.org>
> > ---
> > + * Driver for s5k4ecgx (5MP Camera) from SAMSUNG
> > + * a quarter-inch optical format 1.4 micron 5 megapixel (Mp)
> > + * CMOS image sensor, as reffering to s5k6aa.c
>
> I think this should be mentioned after your own copyright notice,
> e.g. in form of:
>
> Based on s5k6aa driver,
> Copyright (C) 2011, Samsung Electronics Co., Ltd.

ditto

> > + *
> > + * Copyright (C) 2012, Linaro, Sangwook Lee<sangwook.lee@linaro.org>
> > + * Copyright (C) 2012, Insignal Co,. Ltd,  Homin
> > Lee<suapapa@insignal.co.kr>
> > + * Copyright (C) 2011, SAMSUNG ELECTRONICS
>
> No need to shout, "Samsung Electronics Co., Ltd." would be just enough.

ditto

>

> > +#include<media/v4l2-device.h>
> > +#include<media/v4l2-subdev.h>
> > +#include<media/media-entity.h>
> > +#include<media/v4l2-ctrls.h>
> > +#include<media/v4l2-mediabus.h>
> > +#include<media/s5k4ecgx.h>
>
> Can we, please, have these sorted alphabetically ?

OK, I will fix this.

>
> > +#include "s5k4ecgx_regs.h"
> > +
> > +static int debug;
> > +module_param(debug, int, 0644);
> > +
[snip]
> > +/* General purpose parameters */
> > +#define REG_USER_BRIGHTNESS          0x7000022C /* Brigthness */
>
> availble -> available

ditto

>
> > +#define REG_USER_SHARP1                      0x70000A28
> > +#define REG_USER_SHARP2                      0x70000ADE
> > +#define REG_USER_SHARP3                      0x70000B94
> > +#define REG_USER_SHARP4                      0x70000C4A
> > +#define REG_USER_SHARP5                      0x70000D00
> > +
> > +#define LSB(X) (((X)&  0xFF))
> > +#define MSB(Y) (((Y)>>  8)&  0xFF)
>
> Lower case for hex numbers is preferred.
>

ditto

> > +
> > +/*
> > + * Preview size lists supported by sensor
> > + */
> > +struct regval_list *pview_size[] = {
> > +     s5k4ecgx_176_preview,
> > +     s5k4ecgx_352_preview,
> > +     s5k4ecgx_640_preview,
> > +     s5k4ecgx_720_preview,
> > +};
> > +
> > +struct s5k4ecgx_framesize {
> > +     u32 idx; /* Should indicate index of pview_size */
> > +     u32 width;
> > +     u32 height;
> > +};
> > +
> > +/*
> > + * TODO: currently only preview is supported and snapshopt(capture)
> > + * is not implemented yet
> > + */
> > +static struct s5k4ecgx_framesize p_sets[] = {
>
> p_sets -> s5k4ecgx_framesizes ?

Hmm, the structure name needs to be changed properly.

>
> > +     {0, 176, 144},
> > +     {1, 352, 288},
> > +     {2, 640, 480},
> > +     {3, 720, 480},
>
> I believe we can do without presets for just the preview operation mode.

This (p_sets[]) is only used to configure preview size dynamically
when _s_fmt is called and then s_stream become on.

> Then, the number of registers to configure when device is powered on
> should then decrease significantly.
> Those presets are meant to speed up switching the device context, e.g.
> from preview to capture, but they just slow down initialization, because
> you have to configure all presets beforehand.
>
> > +};
> > +
> > +#define S5K4ECGX_NUM_PREV ARRAY_SIZE(p_sets)
> > +struct s5k4ecgx_pixfmt {
> > +     enum v4l2_mbus_pixelcode code;
> > +     u32 colorspace;
> > +};
> > +
> > +/* By defaut value, output from sensor will be YUV422 0-255 */
> > +static const struct s5k4ecgx_pixfmt s5k4ecgx_formats[] = {
> > +     { V4L2_MBUS_FMT_YUYV8_2X8, V4L2_COLORSPACE_JPEG},
>
> Nit: missing whitespace before }.

Thanks, I will fix this.

>
> > +};
> > +
> > +struct s5k4ecgx_preset {
> > +             /* output pixel format and resolution */
> > +             struct v4l2_mbus_framefmt mbus_fmt;
> > +             u8 clk_id;
> > +             u8 index;
> > +};
> > +
> > +struct s5k4ecgx {
> > +     struct v4l2_subdev sd;
> > +     struct media_pad pad;
> > +     struct v4l2_ctrl_handler handler;
> > +
> > +     struct s5k4ecgx_platform_data *pdata;
> > +     struct s5k4ecgx_preset presets[S5K4ECGX_MAX_PRESETS];
> > +     struct s5k4ecgx_preset *preset;
> > +     struct s5k4ecgx_framesize *p_now;       /* Current frame size */
> > +     struct v4l2_fract timeperframe;
> > +
> > +       /* protects the struct members below */
> > +     struct mutex lock;
> > +     int streaming;
> > +
> > +     /* Token for I2C burst write */
> > +     enum token_type reg_type;
> > +     u16 reg_addr_high;
> > +     u16 reg_addr_low;
> > +
> > +     /* Platform specific field */
> > +     int (*set_power)(int);
>
> This need to be replaced with regulator/GPIO API, if possible.
> Platform data callbacks cannot be supported on device tree platforms,
> hence we really need to avoid such callbacks.

Thanks for reminding me about device tree.
let me update the driver with regulator/GPIO API.

>
> > +     int mdelay;
> > +};
> > +
> > +static inline struct s5k4ecgx *to_s5k4ecgx(struct v4l2_subdev *sd)
> > +{
> > +     return container_of(sd, struct s5k4ecgx, sd);
> > +}
> > +
> > +static int s5k4ecgx_write_i2c(struct i2c_client *client, u8 *data, u16
> > len)
> > +{
> > +     struct i2c_msg msg = {client->addr, 0, len, (u8 *)data};
> > +     int ret;
> > +
> > +     ret = i2c_transfer(client->adapter,&msg, 1);
> > +     mdelay(S5K4ECGX_POLL_TIME);
>
> Argh, why is this needed ? If it can't be avoided, please replace it
> with usleep_range().

Ok, I will update this.

>
> > +     if (ret<  0) {
> > +             dev_err(&client->dev, "Failed to write I2C err\n");
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int s5k4ecgx_write32(struct i2c_client *client,
> > +                                    u16 addr, u16 data)
> > +{
> > +     u8 buf[4];
> > +
> > +     buf[0] = MSB(addr); /* SWAP 16 bit */
> > +     buf[1] = LSB(addr);
> > +     buf[2] = MSB(data);
> > +     buf[3] = LSB(data);
> > +
> > +     return s5k4ecgx_write_i2c(client, buf, 4);
> > +}
> > +
> > +static int s5k4ecgx_read_setup(struct v4l2_subdev *sd, u32 addr)
> > +{
> > +     struct i2c_client *client = v4l2_get_subdevdata(sd);
> > +     struct s5k4ecgx *priv = to_s5k4ecgx(sd);
> > +     u16 high = addr>>  16, low =  addr&  0xFFFF;
> > +     int ret = 0;
> > +
> > +     if (priv->reg_type != TOK_READ) {
> > +             priv->reg_addr_high = 0;
> > +             priv->reg_type = TOK_READ;
> > +     }
> > +     if (priv->reg_addr_high != high) {
> > +             ret = s5k4ecgx_write32(client, 0x002C, high);
> > +             priv->reg_addr_high = high;
> > +     }
> > +     ret |= s5k4ecgx_write32(client, 0x002E, low);
> > +
> > +     return ret;
> > +}
> > +
> > +static int s5k4ecgx_read16(struct i2c_client *client, u16 *val)
> > +{
> > +     struct i2c_msg msg[2];
> > +     u16 subaddr = 0x0F12;
> > +     u8 buf[2];
> > +     int err;
> > +
> > +     subaddr = swab16(subaddr);
> > +
> > +     msg[0].addr = client->addr;
> > +     msg[0].flags = 0;
> > +     msg[0].len = 2;
> > +     msg[0].buf = (u8 *)&subaddr;
> > +
> > +     msg[1].addr = client->addr;
> > +     msg[1].flags = I2C_M_RD;
> > +     msg[1].len = 2;
> > +     msg[1].buf = buf;
> > +
> > +     err = i2c_transfer(client->adapter, msg, 2);
> > +     if (unlikely(err != 2)) {
>
> "unlikely" doesn't make much sense here, it's not a fast path.

OK, I will fix this.

>
> > +             dev_err(&client->dev, "Failed to read register 0x%02x!\n",
> > +                     subaddr);
> > +             return -EIO;
> > +     }
> > +
> > +     *val = ((buf[0]<<  8) | buf[1]);
> > +
> > +     return 0;
> > +}
> > +
> > +/*
> > + * Access address will be remapped inside sensor (ARM7 core)
> > + */
> > +static int s5k4ecgx_write_setup(struct v4l2_subdev *sd, u16 high, u16
> > low)
> > +{
> > +     struct i2c_client *client = v4l2_get_subdevdata(sd);
> > +     struct s5k4ecgx *priv = to_s5k4ecgx(sd);
> > +     int ret = 0;
> > +
> > +     if (priv->reg_type != TOK_WRITE) {
> > +             priv->reg_addr_high = 0;
> > +             priv->reg_addr_low = 0;
> > +             priv->reg_type = TOK_WRITE;
> > +     }
> > +
> > +     /* FIXME: no information about 0x0028 in the datasheet */
>
> Hint: have a look at the S5K6AAFX sensor documentation.
>
> > +     if (priv->reg_addr_high != high) {
> > +             ret = s5k4ecgx_write32(client, 0x0028, high);
> > +             priv->reg_addr_high = high;
> > +             priv->reg_addr_low = 0;
> > +     }
> > +
> > +     /* FIXME: no information about 0x002A in the datasheet */
>
> Ditto.
>
> > +     if (priv->reg_addr_low != low) {
> > +             ret |= s5k4ecgx_write32(client, 0x002A, low);
> > +             priv->reg_addr_low = low;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static int s5k4ecgx_write_ctrl(struct v4l2_subdev *sd, u32 addr, u16
> > data)
> > +{
> > +     struct i2c_client *client = v4l2_get_subdevdata(sd);
> > +     int ret;
> > +
> > +     v4l2_dbg(1, debug, sd, "Ctrl register val 0x%4x\n", data);
> > +     s5k4ecgx_write_setup(sd, addr>>  16, addr&  0xffff);
> > +     ret = s5k4ecgx_write32(client, 0x0F12, data);
>
> Just do:
>         return s5k4ecgx_write32(client, 0x0f12, data);

OK, I will change this.

> > +
> > +     return ret;
> > +}
> > +
> > +static u8 *s5k4ecgx_prep_buffer(int *cnt, struct regval_list **pos)
> > +{
> > +     struct regval_list *p_cur = *pos;
> > +     int burst_len = 0, len;
> > +     u8 *p_buf;
> > +
> > +     while (p_cur->type != TOK_TERM) {
> > +             /*
> > +              * Make sure two bytes data are used and address is
> > continous
>
> continous -> continuous

ditto

>
> > +              * to write them in a block
> > +              */
> > +             burst_len += 2;
> > +             if (TOK_WRITE != (p_cur + 1)->type)
> > +                     break;
> > +             if ((p_cur->addr + 2) != (p_cur + 1)->addr)
> > +                     break;
> > +             p_cur += 1;
> > +     }
> > +     p_buf = vmalloc(burst_len + 2);
>
> what ? why ? Is it just to append 2 bytes at the beginning of
> a register address/value array,

Yes it is just to append 2 bytes in the beginning.

>or just to swap regiter value
> bytes ?
>
> > +     if (!p_buf)
> > +             return NULL;
> > +
> > +     p_buf[0] = 0x0F; /* FIXME: no information in the datasheet */
> > +     p_buf[1] = 0x12;
>
> This may be some command write buffer I2C sub-address.

It makes sense. thanks!

>
> > +     p_cur = *pos;
> > +     len = 2;
> > +     burst_len += 2; /* Add two bytes */
> > +     while (1) {
> > +             p_buf[len] = MSB(p_cur->val);
> > +             p_buf[len + 1] = LSB(p_cur->val);
> > +             len += 2;
> > +             if (len<  burst_len)
> > +                     p_cur++;
> > +             else
> > +                     break;
> > +     }
> > +     *pos = p_cur;
> > +     *cnt = burst_len ;
>
> This looks really suspicious to me. I guess the purpose of this
> function is similar to what s5k6aa_write_array() does.
> It looks like a helper for writing an array of registers with
> contiguous addresses.

I think so. but I can't find any burst I2C writing information in the datasheet,
and as long as the sensor requires lots of initial settings value, It makes
sense.

>This shouldn't be needed as long as you update
> the I2C register write pointer as you go, when a difference of two
> subsequent addresses does not equal some constant value.
>
> Can you shed some light on why this function is needed ?
>
currently it is also updating register write pointers (regval_list **pos)
as it is only filling register data except address.

> > +
> > +     return p_buf;
> > +}
> > +
> > +static int s5k4ecgx_write_burst(struct v4l2_subdev *sd,
> > +                                     struct regval_list **pos)
> > +{
> > +     struct s5k4ecgx *priv = to_s5k4ecgx(sd);
> > +     struct i2c_client *client = v4l2_get_subdevdata(sd);
> > +     struct regval_list *p_cur = *pos;
> > +     int burst_len, ret = 0;
> > +     u8 *p_buf;
> > +
> > +     /* Select starting address of burst data */
> > +     s5k4ecgx_write_setup(sd, p_cur->addr>>  16, p_cur->addr&  0xffff);
> > +
> > +     /* Make buffer and then copy data into it */
> > +     p_buf = s5k4ecgx_prep_buffer(&burst_len, pos);
> > +     if (!p_buf)
> > +             return -ENOMEM;
> > +     ret = s5k4ecgx_write_i2c(client, p_buf, burst_len);
> > +     vfree(p_buf);
> > +     priv->reg_addr_low = 0;
> > +
> > +     return ret;
> > +}
> > +
> > +static int s5k4ecgx_write_array(struct v4l2_subdev *sd,
> > +                                     struct regval_list *vals)
> > +{
> > +     struct i2c_client *client = v4l2_get_subdevdata(sd);
> > +     int err = 0;
> > +
> > +     while (vals->type != TOK_TERM&&  !err) {
> > +             switch (vals->type) {
> > +             case TOK_WRITE:
> > +                     /*
> > +                      * This function continues to check the
> > +                      * following addresses, then update the address of
> > vals
> > +                      */
> > +                     err = s5k4ecgx_write_burst(sd,&vals);
> > +                     break;
> > +             case TOK_CMD:
> > +                     err = s5k4ecgx_write32(client, vals->addr,
> > vals->val);
> > +                     break;
> > +             case TOK_DELAY:
> > +                     msleep(vals->val);
> > +                     break;
> > +             default:
> > +                     v4l2_err(sd, "Failed to detect i2c type!\n");
> > +                     err = -EINVAL;
> > +                     break;
> > +             }
> > +             vals++;
> > +     }
> > +
> > +     if (unlikely(vals->type != TOK_TERM) || err)
> > +             v4l2_err(sd, "Failed to write array!\n");
> > +
> > +     return err;
> > +}
> > +
> > +static void s5k4ecgx_init_parameters(struct v4l2_subdev *sd)
> > +{
> > +     struct s5k4ecgx *priv = to_s5k4ecgx(sd);
> > +     int err = 0;
> > +
> > +     priv->streaming = 0;
> > +     /* brigthness default */
> > +     err = s5k4ecgx_write_array(sd, s5k4ecgx_ev_default);
> > +     /* no image effect */
> > +     err |= s5k4ecgx_write_array(sd, s5k4ecgx_effect_normal);
> > +     /* white blance auto */
> > +     err |= s5k4ecgx_write_array(sd, s5k4ecgx_wb_auto);
> > +     err |= s5k4ecgx_write_array(sd, s5k4ecgx_contrast_default);
> > +     err |= s5k4ecgx_write_array(sd, s5k4ecgx_iso_auto);
> > +     /* default 30 FPS */
> > +     err |= s5k4ecgx_write_array(sd, s5k4ecgx_fps_30);
> > +     err |= s5k4ecgx_write_array(sd, s5k4ecgx_scene_default);
> > +     err |= s5k4ecgx_write_array(sd, s5k4ecgx_saturation_default);
> > +     err |= s5k4ecgx_write_array(sd, s5k4ecgx_sharpness_default);
>
> I hope most of these go away, when we're done ;
>
> > +     if (err)
> > +             v4l2_err(sd, "Failed to write init params!\n");
> > +}
> > +
> > +static void s5k4ecgx_set_framesize(struct v4l2_subdev *sd,
> > +                                     struct v4l2_mbus_framefmt *in)
> > +{
> > +     struct s5k4ecgx *priv = to_s5k4ecgx(sd);
> > +     struct s5k4ecgx_framesize *a = p_sets;
> > +     struct s5k4ecgx_framesize *z = p_sets + S5K4ECGX_NUM_PREV - 1;
> > +
> > +     /* If not match, assign the biggest size  */
> > +     while (a != z) {
> > +             if (a->width == in->width&&  a->height == in->height)
> > +                     break;
> > +             a++;
> > +     }
> > +     priv->p_now = a;
> > +}
> > +
> > +static int s5k4ecgx_get_pixfmt_index(struct s5k4ecgx *priv,
> > +                                struct v4l2_mbus_framefmt *mf)
> > +{
> > +     unsigned int i;
> > +
> > +     for (i = 0; i<  ARRAY_SIZE(s5k4ecgx_formats); i++)
> > +             if (mf->colorspace == s5k4ecgx_formats[i].colorspace&&
> > +                 mf->code == s5k4ecgx_formats[i].code)
> > +                     return i;
> > +
> > +     return 0;
> > +}
> > +
> > +static void s5k4ecgx_try_fmt(struct s5k4ecgx *priv,
> > +                          struct v4l2_mbus_framefmt *mf)
> > +{
> > +     unsigned int index;
> > +
> > +     v4l_bound_align_image(&mf->width, S5K4ECGX_WIN_WIDTH_MIN,
> > +                     S5K4ECGX_WIN_WIDTH_MAX, 1,&mf->height,
> > +                     S5K4ECGX_WIN_HEIGHT_MIN, S5K4ECGX_WIN_HEIGHT_MAX,
> > 1, 0);
>
> You should return just 1 of 4 discrete resolutions here, according to
> p_sets[] array. Now the user is juts fooled that we support any resolution
> in range from S5K4ECGX_WIN_WIDTH_MIN x S5K4ECGX_WIN_HEIGHT_MIN to
> S5K4ECGX_WIN_WIDTH_MAX x S5K4ECGX_WIN_HEIGHT_MAX, with 2 pixel increments.
>
> Please see noon010pc30.c driver for a reference.
Thanks for info.
>
> > +     if (mf->colorspace != V4L2_COLORSPACE_JPEG)
> > +             mf->colorspace = V4L2_COLORSPACE_JPEG;
> > +     index = s5k4ecgx_get_pixfmt_index(priv, mf);
> > +     mf->colorspace  = s5k4ecgx_formats[0].colorspace;
> > +     mf->code                = s5k4ecgx_formats[0].code;
> > +     mf->field               = V4L2_FIELD_NONE;
> > +}
> > +
> > +static int s5k4ecgx_read_fw_ver(struct v4l2_subdev *sd)
> > +{
> > +     struct i2c_client *client = v4l2_get_subdevdata(sd);
> > +     u16 fw_ver = 0, hw_rev = 0;
> > +
> > +     s5k4ecgx_read_setup(sd, REG_FW_VERSION);
> > +     s5k4ecgx_read16(client,&fw_ver);
> > +     if (fw_ver != S5K4ECGX_FW_VERSION) {
> > +             v4l2_err(sd, "FW version check failed!");
> > +             return -ENODEV;
> > +     }
> > +     s5k4ecgx_read_setup(sd, REG_FW_REVISION);
> > +     s5k4ecgx_read16(client,&hw_rev);
> > +
> > +     if (hw_rev == S5K4ECGX_REVISION_1_1) {
> > +             v4l2_info(sd, "chip found FW ver: 0x%X, HW rev: 0x%X\n",
> > +                                             fw_ver, hw_rev);
> > +     } else {
> > +             v4l2_err(sd, "chip found but it has unknown revision
> > 0x%x\n",
>
> How about just making it:
>                 v4l2_err(sd, "Unknown H/W revision: %#x\n", hw_rev);
>
Oh, it looks better, I will take it.

> > +             return -ENODEV;
> > +     };
> > +
> > +     return 0;
> > +}
> > +
> > +static int s5k4ecgx_enum_mbus_code(struct v4l2_subdev *sd,
> > +                                struct v4l2_subdev_fh *fh,
> > +                                struct v4l2_subdev_mbus_code_enum
> > *code)
> > +{
> > +     if (code->index>= ARRAY_SIZE(s5k4ecgx_formats))
> > +             return -EINVAL;
> > +     code->code = s5k4ecgx_formats[code->index].code;
> > +
> > +     return 0;
> > +}
> > +
> > +static int s5k4ecgx_enum_frame_size(struct v4l2_subdev *sd,
> > +                               struct v4l2_subdev_fh *fh,
> > +                               struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > +     int i = ARRAY_SIZE(s5k4ecgx_formats);
> > +
> > +     if (fse->index>  0)
>
> No, if you support 4 resolutions, fse->index = 0..3 are valid.

Thanks, let me fix this.

>
> > +             return -EINVAL;
> > +
> > +     while (--i)
> > +             if (fse->code == s5k4ecgx_formats[i].code)
> > +                     break;
> > +
> > +     fse->code = s5k4ecgx_formats[i].code;
> > +     fse->min_width  = S5K4ECGX_WIN_WIDTH_MIN;
> > +     fse->max_width  = S5K4ECGX_WIN_WIDTH_MAX;
>
> min_width, max_width should be equal in your case, to a pixel width
> of resolution pointed by fse->index.
>
> > +     fse->max_height = S5K4ECGX_WIN_HEIGHT_MIN;
> > +     fse->min_height = S5K4ECGX_WIN_HEIGHT_MAX;
>
> Similar as for width.
>
> > +
> > +     return 0;
> > +}
> > +
> > +static int s5k4ecgx_get_fmt(struct v4l2_subdev *sd, struct
> > v4l2_subdev_fh *fh,
> > +                         struct v4l2_subdev_format *fmt)
> > +{
> > +     struct s5k4ecgx *priv = to_s5k4ecgx(sd);
> > +     struct v4l2_mbus_framefmt *mf;
> > +
> > +     memset(fmt->reserved, 0, sizeof(fmt->reserved));
>
> I think it's safe to drop that line.
>
> > +     if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > +             mf = v4l2_subdev_get_try_format(fh, 0);
> > +             fmt->format = *mf;
> > +             return 0;
> > +     }
> > +     mutex_lock(&priv->lock);
> > +     fmt->format.width = priv->p_now->width;
> > +     fmt->format.height = priv->p_now->height;
> > +     mutex_unlock(&priv->lock);
> > +
> > +     return 0;
> > +}
> > +
> > +static int s5k4ecgx_set_fmt(struct v4l2_subdev *sd, struct
> > v4l2_subdev_fh *fh,
> > +                         struct v4l2_subdev_format *fmt)
> > +{
> > +     struct s5k4ecgx *priv = to_s5k4ecgx(sd);
> > +     struct s5k4ecgx_preset *preset = priv->preset;
> > +     struct v4l2_mbus_framefmt *mf;
> > +     struct v4l2_rect *crop;
> > +     int ret = 0;
> > +
> > +     mutex_lock(&priv->lock);
> > +     s5k4ecgx_try_fmt(priv,&fmt->format);
> > +
> > +     if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > +             mf = v4l2_subdev_get_try_format(fh, fmt->pad);
> > +             crop = v4l2_subdev_get_try_crop(fh, 0);
> > +     } else {
> > +             if (priv->streaming)
> > +                     ret = -EBUSY;
> > +             else
> > +                     mf =&preset->mbus_fmt;
> > +     }
> > +     if (ret == 0) {
> > +             *mf = fmt->format;
> > +             s5k4ecgx_set_framesize(sd,&fmt->format);
> > +     }
> > +     mutex_unlock(&priv->lock);
> > +
> > +     return ret;
> > +}
> > +
> > +static const struct v4l2_subdev_pad_ops s5k4ecgx_pad_ops = {
> > +     .enum_mbus_code         = s5k4ecgx_enum_mbus_code,
> > +     .enum_frame_size        = s5k4ecgx_enum_frame_size,
> > +     .get_fmt        = s5k4ecgx_get_fmt,
> > +     .set_fmt        = s5k4ecgx_set_fmt,
> > +};
> > +
> > +/*
> > + * V4L2 subdev controls
> > + */
> > +static int s5k4ecgx_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +
> > +     struct v4l2_subdev *sd =&container_of(ctrl->handler, struct
> > s5k4ecgx,
> > +                                             handler)->sd;
> > +     struct s5k4ecgx *priv = to_s5k4ecgx(sd);
> > +     int err = 0;
> > +
> > +     v4l2_dbg(1, debug, sd, "ctrl: 0x%x, value: %d\n", ctrl->id,
> > ctrl->val);
> > +     mutex_lock(&priv->lock);
> > +
> > +     switch (ctrl->id) {
> > +     case V4L2_CID_CONTRAST:
> > +             err = s5k4ecgx_write_ctrl(sd, REG_USER_CONTRAST,
> > ctrl->val);
> > +             break;
> > +
> > +     case V4L2_CID_SATURATION:
> > +             err = s5k4ecgx_write_ctrl(sd, REG_USER_SATURATION,
> > ctrl->val);
> > +             break;
> > +
> > +     case V4L2_CID_SHARPNESS:
> > +             err |= s5k4ecgx_write_ctrl(sd, REG_USER_SHARP1,
> > ctrl->val);
> > +             err |= s5k4ecgx_write_ctrl(sd, REG_USER_SHARP2,
> > ctrl->val);
> > +             err |= s5k4ecgx_write_ctrl(sd, REG_USER_SHARP3,
> > ctrl->val);
> > +             err |= s5k4ecgx_write_ctrl(sd, REG_USER_SHARP4,
> > ctrl->val);
> > +             err |= s5k4ecgx_write_ctrl(sd, REG_USER_SHARP5,
> > ctrl->val);
>
> ????????
>
> Are you writing sharpness value for all possible "presets" even though
> only preset 0 is used ?

I don't think so. If I see the base reference code (from Google Nexus
S 3.0 kernel)
with one set sharpness, driver always write 5 registers continuously
with the defined
same value.

For example:

S5K4ECGX_DEFINE_REGSET(s5k4ecgx_Sharpness_Minus_3) = {
        S5K4ECGX_REG_WRITE(0x70000A28, 0x0000),
        S5K4ECGX_REG_WRITE(0x70000ADE, 0x0000),
        S5K4ECGX_REG_WRITE(0x70000B94, 0x0000),
        S5K4ECGX_REG_WRITE(0x70000C4A, 0x0000),
        S5K4ECGX_REG_WRITE(0x70000D00, 0x0000),
        S5K4ECGX_REGSET_END
};


>
> > +             break;
> > +
> > +     case V4L2_CID_BRIGHTNESS:
> > +             err = s5k4ecgx_write_ctrl(sd, REG_USER_BRIGHTNESS,
> > ctrl->val);
> > +             break;
> > +     default:
>
> This should never happen, i.e. s_ctrl is only called with valid ctrl->id.

Thanks, I will fix this.

>
> > +             v4l2_dbg(1, debug, sd, "unknown set ctrl id 0x%x\n",
> > ctrl->id);
> > +             err = -ENOIOCTLCMD;
>
> Return value should be -EINVAL, if you ever decide to keep it.
>
> > +             break;
> > +     }
> > +
> > +     /* Review this */
> > +     priv->reg_type = TOK_TERM;
> > +
> > +     mutex_unlock(&priv->lock);
> > +
> > +     if (err<  0)
> > +             v4l2_err(sd, "Failed to write videoc_s_ctrl err %d\n",
> > err);
>
> "Failed to write videoc_s_ctrl err" ? ;)
>
> > +     return err;
> > +}
> > +
> > +static const struct v4l2_ctrl_ops s5k4ecgx_ctrl_ops = {
> > +     .s_ctrl = s5k4ecgx_s_ctrl,
> > +};
> > +
> > +/*
> > + * Reading s5k4ecgx version information
> > + */
> > +static int s5k4ecgx_registered(struct v4l2_subdev *sd)
> > +{
> > +     struct s5k4ecgx *priv = to_s5k4ecgx(sd);
> > +     int ret;
> > +
> > +     if (!priv->set_power) {
> > +             v4l2_err(sd, "Error: power callback undefined!\n");
>
> Is it possible to avoid this callback, by moving voltage regulators
> handling to this driver ?

let me see it again when I use voltage regulator

>
> > +             return -EIO;
> > +     }
> > +
> > +     mutex_lock(&priv->lock);
> > +     priv->set_power(true);
> > +     /* Time to stablize sensor */
> > +     mdelay(priv->mdelay);
>
> Ugh, why people keep using those busy wait delays, instead of
> yielding CPU cycles to other tasks, by using *sleep*(..) versions ?
>
> > +     ret = s5k4ecgx_read_fw_ver(sd);
> > +     priv->set_power(false);
> > +     mutex_unlock(&priv->lock);
> > +
> > +     return ret;
> > +}
> > +
> > +/*
> > + *  V4L2 subdev internal operations
> > + */
> > +static int s5k4ecgx_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> > *fh)
> > +{
> > +
> > +     struct v4l2_mbus_framefmt *format = v4l2_subdev_get_try_format(fh,
> > 0);
> > +     struct v4l2_rect *crop = v4l2_subdev_get_try_crop(fh, 0);
> > +
> > +     format->colorspace = s5k4ecgx_formats[0].colorspace;
> > +     format->code = s5k4ecgx_formats[0].code;
> > +     format->width = S5K4ECGX_OUT_WIDTH_DEF;
> > +     format->height = S5K4ECGX_OUT_HEIGHT_DEF;
> > +     format->field = V4L2_FIELD_NONE;
> > +
> > +     crop->width = S5K4ECGX_WIN_WIDTH_MAX;
> > +     crop->height = S5K4ECGX_WIN_HEIGHT_MAX;
> > +     crop->left = 0;
> > +     crop->top = 0;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_internal_ops
> > s5k4ecgx_subdev_internal_ops = {
> > +     .registered = s5k4ecgx_registered,
> > +     .open = s5k4ecgx_open,
> > +};
> > +
> > +static int s5k4ecgx_s_power(struct v4l2_subdev *sd, int on)
> > +{
> > +     struct s5k4ecgx *priv = to_s5k4ecgx(sd);
> > +
> > +     if (!priv->set_power)
> > +             return -EIO;
> > +
> > +     v4l2_dbg(1, debug, sd, "Switching %s\n", on ? "on" : "off");
> > +
> > +     if (on) {
> > +             priv->set_power(on);
> > +             /* Time to stablize sensor */
> > +             mdelay(priv->mdelay);
>
> msleep()
ditto

>
> > +             /* Loading firmware into ARM7 core of sensor */
> > +             if (s5k4ecgx_write_array(sd, s5k4ecgx_init_regs)<  0) {
>
> Yes, those register arrays could be converted to real firmware blobs
> as well. :)
For me, it looks like loading firmware. too many initial values. :-)

>
> > +                     priv->set_power(0); /* Turn off power */
> > +                     return -EIO;
> > +             }
> > +             s5k4ecgx_init_parameters(sd);
> > +     } else {
> > +             priv->set_power(0);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int s5k4ecgx_log_status(struct v4l2_subdev *sd)
> > +{
> > +     v4l2_ctrl_handler_log_status(sd->ctrl_handler, sd->name);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_core_ops s5k4ecgx_core_ops = {
> > +     .s_power = s5k4ecgx_s_power,
> > +     .log_status     = s5k4ecgx_log_status,
> > +};
> > +
> > +static int __s5k4ecgx_s_stream(struct v4l2_subdev *sd, int on)
> > +{
> > +     struct s5k4ecgx *priv = to_s5k4ecgx(sd);
> > +     int err = 0;
> > +
> > +     if (on)
> > +             err = s5k4ecgx_write_array(sd,
> > pview_size[priv->p_now->idx]);
> > +
> > +     return err;
> > +}
> > +
> > +static int s5k4ecgx_s_stream(struct v4l2_subdev *sd, int on)
> > +{
> > +     struct s5k4ecgx *priv = to_s5k4ecgx(sd);
> > +     int ret = 0;
> > +
> > +     v4l2_dbg(1, debug, sd, "Turn streaming %s\n", on ? "on" : "off");
> > +     mutex_lock(&priv->lock);
> > +
> > +     if (on) {
> > +             /* Ignore if s_stream is called twice */
> > +             if (!priv->streaming) {
> > +                     ret = __s5k4ecgx_s_stream(sd, on);
> > +                     if (!ret)
> > +                             priv->streaming = on;
> > +             }
> > +     } else {
> > +             priv->streaming = 0;
>
> Data streaming is still active, as no registers were touched in this
> case. So this line should probably be moved to s_power(sd, 0) case above.

I agree that is more sensible. I just wanted a status variable in the
same function.
let me update this as well.

>
> > +     }
> > +
> > +     mutex_unlock(&priv->lock);
> > +
> > +     return ret;
> > +}
> > +
> > +static const struct v4l2_subdev_video_ops s5k4ecgx_video_ops = {
> > +     .s_stream = s5k4ecgx_s_stream,
> > +};
> > +
> > +static const struct v4l2_subdev_ops s5k4ecgx_ops = {
> > +     .core =&s5k4ecgx_core_ops,
> > +     .pad =&s5k4ecgx_pad_ops,
> > +     .video =&s5k4ecgx_video_ops,
> > +};
> > +
> > +static int s5k4ecgx_initialize_ctrls(struct s5k4ecgx *priv)
> > +{
> > +     const struct v4l2_ctrl_ops *ops =&s5k4ecgx_ctrl_ops;
> > +     struct v4l2_ctrl_handler *hdl =&priv->handler;
> > +     int ret;
> > +
> > +     ret =  v4l2_ctrl_handler_init(hdl, 16);
>
> The hint should be 4, not 16. There are only 4 controls added to the
> control handler below.

Good point, I will change this.

>
> > +     if (ret)
> > +             return ret;
> > +
> > +     v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS, -208, 127, 1, 0);
>
> Is minimum brightness really -208 ?

so to speak, this is kind of reverse engineering :-), not from the datasheet.

I got the values such 0xFF30, 0xFFA0, 0xFFC8, 0XFFE0, 0x0, 0x20, 0X7F
                                 -208      -96
                           127

For example)
S5K4ECGX_DEFINE_REGSET(s5k4ecgx_EV_Minus_4) = {
        S5K4ECGX_REG_WRITE(0x70000230, 0xFF30),
        S5K4ECGX_REGSET_END
};


>
> > +     v4l2_ctrl_new_std(hdl, ops, V4L2_CID_CONTRAST, -127, 127, 1, 0);
> > +     v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SATURATION, -127, 127, 1, 0);
> > +
> > +     /* For sharpness, 0x6024 is default value */
> > +     v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SHARPNESS, -32704, 24612,
> > 8208,
> > +                       24612);
>
> How about:
>         v4l2_ctrl_new_std(hdl, ops, V4L2_CID_SHARPNESS, -32704/8208,
> 24612/8208, 1,
> > +                       24612/8208);
>
> and multiplying brightness control value by 8208 in s_ctrl() ?
> It's not that difficult and would let us present more uniform interface
> to the user.

Good point, I will change this.


> > +     if (hdl->error) {
> > +             ret = hdl->error;
> > +             v4l2_ctrl_handler_free(hdl);
> > +             return ret;
> > +     }
> > +     priv->sd.ctrl_handler = hdl;
> > +
> > +     return 0;
> > +};
> > +
> > +/*
> > + * Set initial values for all preview presets
> > + */
> > +static void s5k4ecgx_presets_data_init(struct s5k4ecgx *priv)
> > +{
> > +     struct s5k4ecgx_preset *preset =&priv->presets[0];
> > +     int i;
> > +
> > +     for (i = 0; i<  S5K4ECGX_MAX_PRESETS; i++) {
> > +             preset->mbus_fmt.width  = S5K4ECGX_OUT_WIDTH_DEF;
> > +             preset->mbus_fmt.height = S5K4ECGX_OUT_HEIGHT_DEF;
> > +             preset->mbus_fmt.code   = s5k4ecgx_formats[0].code;
> > +             preset->index           = i;
> > +             preset->clk_id          = 0;
> > +             preset++;
> > +     }
> > +     priv->preset =&priv->presets[0];
> > +}
> > +
> > +/*
> > +  * Fetching platform data is being done with s_config subdev call.
>
> This is not true, please remove it. There have been no s_config callback
> for ages now.
>
> > +  * In probe routine, we just register subdev device
>
> This comment also doesn't add any special value, IMHO.
>
> > +  */
> > +static int s5k4ecgx_probe(struct i2c_client *client,
> > +                       const struct i2c_device_id *id)
> > +{
> > +     struct v4l2_subdev *sd;
> > +     struct s5k4ecgx *priv;
> > +     struct s5k4ecgx_platform_data *pdata = client->dev.platform_data;
> > +     int     ret;
> > +
> > +     if (pdata == NULL) {
> > +             dev_err(&client->dev, "platform data is missing!\n");
> > +             return -EINVAL;
> > +     }
> > +     priv = kzalloc(sizeof(struct s5k4ecgx), GFP_KERNEL);
>
> devm_kzalloc ?

Thanks, please let me look at this.

>
> > +
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     mutex_init(&priv->lock);
> > +
> > +     priv->set_power = pdata->set_power;
> > +     priv->mdelay = pdata->mdelay;
> > +
> > +     sd =&priv->sd;
> > +     /* Registering subdev */
> > +     v4l2_i2c_subdev_init(sd, client,&s5k4ecgx_ops);
> > +     strlcpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name));
> > +
> > +     sd->internal_ops =&s5k4ecgx_subdev_internal_ops;
> > +     /* Support v4l2 sub-device userspace API */
> > +     sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +
> > +     priv->pad.flags = MEDIA_PAD_FL_SOURCE;
> > +     sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
> > +     ret = media_entity_init(&sd->entity, 1,&priv->pad, 0);
> > +     if (ret)
> > +             goto out_err;
> > +
> > +     ret = s5k4ecgx_initialize_ctrls(priv);
> > +     s5k4ecgx_presets_data_init(priv);
> > +
> > +     if (ret)
> > +             goto out_err;
> > +
> > +     return 0;
> > +
> > + out_err:
> > +     media_entity_cleanup(&priv->sd.entity);
> > +     kfree(priv);
> > +
> > +     return ret;
> > +}
> > +
> > +static int s5k4ecgx_remove(struct i2c_client *client)
> > +{
> > +     struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +     struct s5k4ecgx *priv = to_s5k4ecgx(sd);
> > +
> > +     mutex_destroy(&priv->lock);
> > +     v4l2_device_unregister_subdev(sd);
> > +     v4l2_ctrl_handler_free(&priv->handler);
>
> If you use devm_kzalloc() in probe(), then this could be:
>
>         v4l2_ctrl_handler_free(sd->ctrl_handler);
>
> > +     media_entity_cleanup(&sd->entity);
> > +     kfree(priv);
>
> ...and this line could be dropped.
>
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct i2c_device_id s5k4ecgx_id[] = {
> > +     { S5K4ECGX_DRIVER_NAME, 0 },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, s5k4ecgx_id);
> > +
> > +static struct i2c_driver v4l2_i2c_driver = {
> > +     .driver = {
> > +             .owner  = THIS_MODULE,
> > +             .name = S5K4ECGX_DRIVER_NAME,
> > +     },
> > +     .probe = s5k4ecgx_probe,
> > +     .remove = s5k4ecgx_remove,
> > +     .id_table = s5k4ecgx_id,
> > +};
> > +
> > +module_i2c_driver(v4l2_i2c_driver);
> > +
> > +MODULE_DESCRIPTION("Samsung S5K4ECGX 5MP SOC camera");
> > +MODULE_AUTHOR("Sangwook Lee<sangwook.lee@linaro.org>");
> > +MODULE_AUTHOR("Seok-Young Jang<quartz.jang@samsung.com>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/media/s5k4ecgx.h b/include/media/s5k4ecgx.h
> > new file mode 100644
> > index 0000000..e041761
> > --- /dev/null
> > +++ b/include/media/s5k4ecgx.h
> > @@ -0,0 +1,29 @@
> > +/*
> > + * S5K4ECGX Platform data header
>
> S5K4ECGX image sensor driver header ?
>
> > + *
> > + * Copyright (C) 2012, Linaro
> > + *
> > + * Copyright (C) 2010, SAMSUNG ELECTRONICS
>
> Plase use lower case, and proper copyright for Samsung is exactly:

Good point.

>
>  "Copyright (C) 2010, Samsung Electronics Co., Ltd."
>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#ifndef S5K4ECGX_H
> > +#define S5K4ECGX_H
> > +
> > +/**
> > + * struct ss5k4ecgx_platform_data- s5k4ecgx driver platform data
> > + * @set_power: an callback to give the chance to turn off/on
> > + *            camera which is depending on the board code
> > + * @mdelay   : delay (ms) needed after enabling power
> > + */
> > +
> > +struct s5k4ecgx_platform_data {
> > +     int (*set_power)(int);
>
> It would be good to replace this callback with something else,
> for the reasons I mentioned above. I think for Origen board we
> could get rid of it by adding (fixed) voltage regulator support
> in this driver and passing SRST, nSTBY GPIOs through this platform
> data structure.

Fair enough, I will fix this.

>
> It's done this way in s5k6aa case, and for some boards the
> set_power callback remains unused (it's treated as optional in
> the driver).
>
> > +     int mdelay;
> > +};
> > +
> > +#endif /* S5K4ECGX_H */
>
> --
>
> Thanks!
> Sylwester


Thanks
Sangwook