Re: [PATCH v5] media: i2c: add support for omnivision's ov2659 sensor

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Prabhakar,

On Thu, Mar 12, 2015 at 11:22:36PM +0000, Lad Prabhakar wrote:
...
> +static int ov2659_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	const struct ov2659_platform_data *pdata = ov2659_get_pdata(client);
> +	struct v4l2_subdev *sd;
> +	struct ov2659 *ov2659;
> +	struct clk *clk;
> +	int ret;
> +
> +	if (!pdata) {
> +		dev_err(&client->dev, "platform data not specified\n");
> +		return -EINVAL;
> +	}
> +
> +	ov2659 = devm_kzalloc(&client->dev, sizeof(*ov2659), GFP_KERNEL);
> +	if (!ov2659)
> +		return -ENOMEM;
> +
> +	ov2659->pdata = pdata;
> +	ov2659->client = client;
> +
> +	clk = devm_clk_get(&client->dev, "xvclk");
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	ov2659->xvclk_frequency = clk_get_rate(clk);
> +	if (ov2659->xvclk_frequency < 6000000 ||
> +	    ov2659->xvclk_frequency > 27000000)
> +		return -EINVAL;
> +
> +	v4l2_ctrl_handler_init(&ov2659->ctrls, 2);
> +	v4l2_ctrl_new_std(&ov2659->ctrls, &ov2659_ctrl_ops,
> +			  V4L2_CID_PIXEL_RATE, ov2659->xvclk_frequency,
> +			  ov2659->xvclk_frequency, 1, ov2659->xvclk_frequency);

ov2659->xvclk_frequency is the frequency of the external clock, not the
pixel rate. If I understand correctly, you should use the value of the
link-frequency property instead (as long as it's one pixel per clock).

With this fixed,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> +	v4l2_ctrl_new_std_menu_items(&ov2659->ctrls, &ov2659_ctrl_ops,
> +				     V4L2_CID_TEST_PATTERN,
> +				     ARRAY_SIZE(ov2659_test_pattern_menu) - 1,
> +				     0, 0, ov2659_test_pattern_menu);
> +	ov2659->sd.ctrl_handler = &ov2659->ctrls;
> +
> +	if (ov2659->ctrls.error) {
> +		dev_err(&client->dev, "%s: control initialization error %d\n",
> +			__func__, ov2659->ctrls.error);
> +		return  ov2659->ctrls.error;
> +	}
> +
> +	sd = &ov2659->sd;
> +	client->flags |= I2C_CLIENT_SCCB;
> +	v4l2_i2c_subdev_init(sd, client, &ov2659_subdev_ops);
> +
> +	sd->internal_ops = &ov2659_subdev_internal_ops;
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +		     V4L2_SUBDEV_FL_HAS_EVENTS;
> +
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	ov2659->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
> +	ret = media_entity_init(&sd->entity, 1, &ov2659->pad, 0);
> +	if (ret < 0) {
> +		v4l2_ctrl_handler_free(&ov2659->ctrls);
> +		return ret;
> +	}
> +#endif
> +
> +	mutex_init(&ov2659->lock);
> +
> +	ov2659_get_default_format(&ov2659->format);
> +	ov2659->frame_size = &ov2659_framesizes[2];
> +	ov2659->format_ctrl_regs = ov2659_formats[0].format_ctrl_regs;
> +
> +	ret = ov2659_detect(sd);
> +	if (ret < 0)
> +		goto error;
> +
> +	/* Calculate the PLL register value needed */
> +	ov2659_pll_calc_params(ov2659);
> +
> +	ret = v4l2_async_register_subdev(&ov2659->sd);
> +	if (ret)
> +		goto error;
> +
> +	dev_info(&client->dev, "%s sensor driver registered !!\n", sd->name);
> +
> +	return 0;
> +
> +error:
> +	v4l2_ctrl_handler_free(&ov2659->ctrls);
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	media_entity_cleanup(&sd->entity);
> +#endif
> +	mutex_destroy(&ov2659->lock);
> +	return ret;
> +}

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk