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

This dirver implements preview mode of the S5K4ECGX sensor.
capture (snapshot) operation, face detection are missing now.

Following controls are supported:
contrast/saturation/birghtness/sharpness

Signed-off-by: Sangwook Lee <sangwook.lee@linaro.org>
---
 drivers/media/video/Kconfig    |    7 +
 drivers/media/video/Makefile   |    1 +
 drivers/media/video/s5k4ecgx.c |  881 ++++++++++++++++++++++++++++++++++++++++
 include/media/s5k4ecgx.h       |   29 ++
 4 files changed, 918 insertions(+)
 create mode 100644 drivers/media/video/s5k4ecgx.c
 create mode 100644 include/media/s5k4ecgx.h

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

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

birghtness -> brightness
 
> Signed-off-by: Sangwook Lee<sangwook.lee@linaro.org>
> ---
>   drivers/media/video/Kconfig    |    7 +
>   drivers/media/video/Makefile   |    1 +
>   drivers/media/video/s5k4ecgx.c |  881 ++++++++++++++++++++++++++++++++++++++++
>   include/media/s5k4ecgx.h       |   29 ++
>   4 files changed, 918 insertions(+)
>   create mode 100644 drivers/media/video/s5k4ecgx.c
>   create mode 100644 include/media/s5k4ecgx.h
> 
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 99937c9..45d7f99 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -559,6 +559,13 @@ config VIDEO_S5K6AA
>   	  This is a V4L2 sensor-level driver for Samsung S5K6AA(FX) 1.3M
>   	  camera sensor with an embedded SoC image signal processor.
> 
> +config VIDEO_S5K4ECGX
> +        tristate "Samsung S5K4ECGX sensor support"
> +        depends on I2C&&  VIDEO_V4L2&&  VIDEO_V4L2_SUBDEV_API
> +        ---help---
> +          This is a V4L2 sensor-level driver for Samsung S5K4ECGX 5M
> +          camera sensor with an embedded SoC image signal processor.
> +
>   source "drivers/media/video/smiapp/Kconfig"
> 
>   comment "Flash devices"
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index d209de0..605bf35 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -79,6 +79,7 @@ obj-$(CONFIG_VIDEO_SR030PC30)	+= sr030pc30.o
>   obj-$(CONFIG_VIDEO_NOON010PC30)	+= noon010pc30.o
>   obj-$(CONFIG_VIDEO_M5MOLS)	+= m5mols/
>   obj-$(CONFIG_VIDEO_S5K6AA)	+= s5k6aa.o
> +obj-$(CONFIG_VIDEO_S5K4ECGX)    += s5k4ecgx.o
>   obj-$(CONFIG_VIDEO_SMIAPP)	+= smiapp/
>   obj-$(CONFIG_VIDEO_ADP1653)	+= adp1653.o
>   obj-$(CONFIG_VIDEO_AS3645A)	+= as3645a.o
> diff --git a/drivers/media/video/s5k4ecgx.c b/drivers/media/video/s5k4ecgx.c
> new file mode 100644
> index 0000000..e277d71
> --- /dev/null
> +++ b/drivers/media/video/s5k4ecgx.c
> @@ -0,0 +1,881 @@
> +/*
> + * 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.

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

> + *
> + * 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.
> + */
> +
> +#include<linux/module.h>
> +#include<linux/i2c.h>
> +#include<linux/delay.h>
> +#include<linux/vmalloc.h>
> +#include<linux/slab.h>
> +
> +#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 ?

> +#include "s5k4ecgx_regs.h"
> +
> +static int debug;
> +module_param(debug, int, 0644);
> +
> +#define S5K4ECGX_DRIVER_NAME		"s5k4ecgx"
> +
> +/* Basic windows sizes */
> +#define S5K4ECGX_OUT_WIDTH_DEF		640
> +#define S5K4ECGX_OUT_HEIGHT_DEF		480
> +#define S5K4ECGX_WIN_WIDTH_MAX		1024
> +#define S5K4ECGX_WIN_HEIGHT_MAX		600
> +#define S5K4ECGX_WIN_WIDTH_MIN		176
> +#define S5K4ECGX_WIN_HEIGHT_MIN		144
> +
> +/* Firmware revision information */
> +#define S5K4ECGX_REVISION_1_1		0x11
> +#define S5K4ECGX_FW_VERSION		0x4EC0
> +#define REG_FW_VERSION			0x700001A4
> +#define REG_FW_REVISION			0x700001A6
> +
> +/* For now we use only one user configuration register set */
> +#define S5K4ECGX_MAX_PRESETS		1
> +
> +/* Review this depending on system */
> +#define S5K4ECGX_POLL_TIME		1 /* ms */
> +
> +/* General purpose parameters */
> +#define REG_USER_BRIGHTNESS		0x7000022C /* Brigthness */
> +#define REG_USER_CONTRAST		0x7000022E /* Contrast */
> +#define REG_USER_SATURATION		0x70000230 /* Saturation */
> +
> +/* FIXME: No information availble about these register from the datasheet */

availble -> available

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

> +
> +/*
> + * 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 ?

> +	{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.
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 }.

> +};
> +
> +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.

> +	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().

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

> +		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);
> +
> +	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

> +		 * 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, 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.

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

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

> +	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);

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

> +		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 ?

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

> +		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 ?

> +		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()

> +		/* 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. :)

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

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

> +	if (ret)
> +		return ret;
> +
> +	v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS, -208, 127, 1, 0);

Is minimum brightness really -208 ?

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

> +	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 ?

> +
> +	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:

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

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

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