Hi David
Thanks for the review!
On 17 July 2012 22:45, David Cohen <david.a.co...@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....@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.
>
>
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);
>> + 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.
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;
>> + else
>>
>
> This "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....@linaro.org>");
>> +MODULE_AUTHOR("Seok-Young Jang <quartz.j...@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 */
>>
>>
>
>
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
