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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux