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@xxxxxxxxxx> --- 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@xxxxxxxxxx>"); +MODULE_AUTHOR("Seok-Young Jang <quartz.jang@xxxxxxxxxxx>"); +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 */
-- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html