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

Hi Sangwook,

I've few comments, some just nitpicking. Feel free to disagree. :)

On 07/17/2012 07:17 PM, Sangwook Lee wrote:

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 | 871 ++++++++++++++++++++++++++++++++++++++++ include/media/s5k4ecgx.h | 29 ++ 4 files changed, 908 insertions(+) create mode 100644 drivers/media/video/s5k4ecgx.c create mode 100644 include/media/s5k4ecgx.h

[snip]

+/*

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

• break;
  

• case V4L2_CID_BRIGHTNESS:

• err = s5k4ecgx_write_ctrl(sd, REG_USER_BRIGHTNESS, ctrl->val);
  

• break;
  

• default:

• v4l2_dbg(1, debug, sd, "unknown set ctrl id 0x%x\n", ctrl->id);
  

• err = -ENOIOCTLCMD;
  

• break;
  

• }
• /* Review this */
• priv->reg_type = TOK_TERM;
• if (err < 0)

• v4l2_err(sd, "Failed to write videoc_s_ctrl err %d\n", err);
  

I like to hold locks only when strictly necessary. You could write this error message after it's released.

• mutex_unlock(&priv->lock);
• 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, "Failed to call power-up function!\n");
  

Maybe it's more accurate to say function isn't set.

• return -EIO;
  

• }
• mutex_lock(&priv->lock);
• priv->set_power(true);
• /* Time to stablize sensor */
• mdelay(priv->mdelay);
• 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 val) +{

• struct s5k4ecgx *priv = to_s5k4ecgx(sd);
• if (!priv->set_power)

• return -EIO;
  

• v4l2_dbg(1, debug, sd, "Switching %s\n", val ? "on" : "off");
• if (val) {

• priv->set_power(val);
  

• /* Time to stablize sensor */
  

• mdelay(priv->mdelay);
  

• /* Loading firmware into ARM7 core of sensor */
  

• if (s5k4ecgx_write_array(sd, s5k4ecgx_init_regs) < 0)
  

• 	return -EIO;
  

Shouldn't you s_power(0) in case of error?

• s5k4ecgx_init_parameters(sd);
  

• } else {

• priv->set_power(val);
  

• }
• 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 && !priv->streaming)

• ret = __s5k4ecgx_s_stream(sd, on);
  

• else

• priv->streaming = 0;
  

Is s_stream(1) is called twice, either you ignore it or return error. But turning it to s_stream(0) isn't correct to me.

• 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);
• if (ret)

• return ret;
  

• v4l2_ctrl_new_std(hdl, ops, V4L2_CID_BRIGHTNESS, -208, 127, 1, 0);
• 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);
  

• 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.
• • In probe routine, we just register subdev device
• */

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

• else

This "else" could be removed.

• 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);
• v4l2_device_unregister_subdev(sd);
• v4l2_ctrl_handler_free(&priv->handler);
• media_entity_cleanup(&sd->entity);
• mutex_destroy(&priv->lock);

For debugging purpose, maybe mutex_destroy() could be first one.

Kind regards.

David Cohen

• kfree(priv);
• 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
• • Copyright (C) 2012, Linaro
• • Copyright (C) 2010, SAMSUNG ELECTRONICS
• • 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);
• int mdelay;

+};

+#endif /* S5K4ECGX_H */