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

[Francesco Lavra <francescolavra.fl@gmail.com>]

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