Re: [RFC PATCH v6] media: add v4l2 subdev driver for S5K4ECGX sensor

Hi, I'm going to report the (few) things which are also present in Sylwester's tree.

On 09/06/2012 05:08 PM, Sangwook Lee wrote:

This patch adds driver for S5K4ECGX sensor with embedded ISP SoC, S5K4ECGX, which is a 5M CMOS Image sensor from Samsung The driver implements preview mode of the S5K4ECGX sensor. capture (snapshot) operation, face detection are missing now. Following controls are supported: contrast/saturation/brightness/sharpness

Signed-off-by: Sangwook Lee sangwook.lee@linaro.org Reviewed-by: Sylwester Nawrocki s.nawrocki@samsung.com

[snip]

+static const char * const s5k4ecgx_supply_names[] = {

• /*

• * Usually 2.8V is used for analog power (vdda)
  

• * and digital IO (vddio, vddd_core)
  

s/vddd_core/vddcore

[snip]

+static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd) +{

• const struct firmware *fw;
• int err, i, regs_num;
• struct i2c_client *client = v4l2_get_subdevdata(sd);
• u16 val;
• u32 addr, crc, crc_file, addr_inc = 0;
• err = request_firmware(&fw, S5K4ECGX_FIRMWARE, sd->v4l2_dev->dev);
• if (err) {

• v4l2_err(sd, "Failed to read firmware %s\n", S5K4ECGX_FIRMWARE);
  

• return err;
  

• }
• regs_num = *(u32 *)(fw->data);
• v4l2_dbg(3, debug, sd, "FW: %s size %d register sets %d\n",

•  S5K4ECGX_FIRMWARE, fw->size, regs_num);
  

• regs_num++; /* Add header */
• if (fw->size != regs_num * FW_RECORD_SIZE + FW_CRC_SIZE) {

• err = -EINVAL;
  

• goto fw_out;
  

• }
• crc_file = *(u32 *)(fw->data + regs_num * FW_RECORD_SIZE);

Depending on the value of regs_num, this may result in unaligned access

[snip]

+static int s5k4ecgx_s_power(struct v4l2_subdev *sd, int on) +{

• struct s5k4ecgx *priv = to_s5k4ecgx(sd);
• int ret;
• v4l2_dbg(1, debug, sd, "Switching %s\n", on ? "on" : "off");
• if (on) {

• ret = __s5k4ecgx_power_on(priv);
  

• if (ret < 0)
  

• 	return ret;
  

• /* Time to stabilize sensor */
  

• msleep(100);
  

• ret = s5k4ecgx_init_sensor(sd);
  

• if (ret < 0)
  

• 	__s5k4ecgx_power_off(priv);
  

• else
  

• 	priv->set_params = 1;
  

• } else {

• ret = __s5k4ecgx_power_off(priv);
  

• }
• return 0;

return ret;

[snip]

+static int s5k4ecgx_probe(struct i2c_client *client,

• 	  const struct i2c_device_id *id)
  

+{

• int ret, i;
• struct v4l2_subdev *sd;
• struct s5k4ecgx *priv;
• struct s5k4ecgx_platform_data *pdata = client->dev.platform_data;
• if (pdata == NULL) {

• dev_err(&client->dev, "platform data is missing!\n");
  

• return -EINVAL;
  

• }
• priv = devm_kzalloc(&client->dev, sizeof(struct s5k4ecgx), GFP_KERNEL);
• if (!priv)

• return -ENOMEM;
  

• mutex_init(&priv->lock);
• priv->streaming = 0;
• 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 user space 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)

• return ret;
  

• ret = s5k4ecgx_config_gpios(priv, pdata);
• if (ret) {

• dev_err(&client->dev, "Failed to set gpios\n");
  

• goto out_err1;
  

• }
• for (i = 0; i < S5K4ECGX_NUM_SUPPLIES; i++)

• priv->supplies[i].supply = s5k4ecgx_supply_names[i];
  

• ret = devm_regulator_bulk_get(&client->dev, S5K4ECGX_NUM_SUPPLIES,

• 		 priv->supplies);
  

• if (ret) {

• dev_err(&client->dev, "Failed to get regulators\n");
  

• goto out_err2;
  

• }
• ret = s5k4ecgx_init_v4l2_ctrls(priv);
• if (ret)

• goto out_err2;
  

• priv->curr_pixfmt = &s5k4ecgx_formats[0];
• priv->curr_frmsize = &s5k4ecgx_prev_sizes[0];
• return 0;

+out_err2:

• s5k4ecgx_free_gpios(priv);

+out_err1:

• media_entity_cleanup(&priv->sd.entity);
• return ret;

+}

+static int s5k4ecgx_remove(struct i2c_client *client) +{

• struct v4l2_subdev *sd = i2c_get_clientdata(client);
• struct s5k4ecgx *priv = to_s5k4ecgx(sd);
• mutex_destroy(&priv->lock);
• v4l2_device_unregister_subdev(sd);
• v4l2_ctrl_handler_free(&priv->handler);
• media_entity_cleanup(&sd->entity);
• return 0;

Maybe s5k4ecgx_free_gpios() should be called?

Regards, Francesco