Hi David
Thanks for the review!
On 17 July 2012 22:45, David Cohen david.a.cohen@linux.intel.com wrote:
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, structs5k4ecgx,
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.
Good point, I will change it.
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.
It makes more sense. I will change comments.
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?
Thanks for this again. I'll add that.
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);elsepriv->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.
Fair enough! I will change it.
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;elseThis "else" could be removed.
Good point. thanks. I will remote it.
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.
Good point, I will change it
Regards Sangwook
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 */