Message-ID: <20180324105748.nwrw2grdqq2hrjlh@valkosipuli.retiisi.org.uk>
Date:   Sat, 24 Mar 2018 12:57:48 +0200
From:   Sakari Ailus <sakari.ailus@....fi>
To:     Rui Miguel Silva <rui.silva@...aro.org>
Cc:     mchehab@...nel.org, sakari.ailus@...ux.intel.com,
        hverkuil@...all.nl, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, Ryan Harkin <ryan.harkin@...aro.org>
Subject: Re: [PATCH v3 2/2] media: ov2680: Add Omnivision OV2680 sensor driver

Hi Rui,

I wanted to go through the code the last time and I'd have a few review
comments below...

On Tue, Mar 13, 2018 at 11:33:11AM +0000, Rui Miguel Silva wrote:
...
> +static int ov2680_gain_set(struct ov2680_dev *sensor, bool auto_gain)
> +{
> +	struct ov2680_ctrls *ctrls = &sensor->ctrls;
> +	u32 gain;
> +	int ret;
> +
> +	ret = ov2680_mod_reg(sensor, OV2680_REG_R_MANUAL, BIT(1),
> +			     auto_gain ? 0 : BIT(1));
> +	if (ret < 0)
> +		return ret;
> +
> +	if (auto_gain || !ctrls->gain->is_new)
> +		return 0;
> +
> +	gain = ctrls->gain->val;
> +
> +	ret = ov2680_write_reg16(sensor, OV2680_REG_GAIN_PK, gain);
> +
> +	return 0;
> +}
> +
> +static int ov2680_gain_get(struct ov2680_dev *sensor)
> +{
> +	u32 gain;
> +	int ret;
> +
> +	ret = ov2680_read_reg16(sensor, OV2680_REG_GAIN_PK, &gain);
> +	if (ret)
> +		return ret;
> +
> +	return gain;
> +}
> +
> +static int ov2680_auto_gain_enable(struct ov2680_dev *sensor)
> +{
> +	return ov2680_gain_set(sensor, true);
> +}
> +
> +static int ov2680_auto_gain_disable(struct ov2680_dev *sensor)
> +{
> +	return ov2680_gain_set(sensor, false);
> +}
> +
> +static int ov2680_exposure_set(struct ov2680_dev *sensor, bool auto_exp)
> +{
> +	struct ov2680_ctrls *ctrls = &sensor->ctrls;
> +	u32 exp;
> +	int ret;
> +
> +	ret = ov2680_mod_reg(sensor, OV2680_REG_R_MANUAL, BIT(0),
> +			     auto_exp ? 0 : BIT(0));
> +	if (ret < 0)
> +		return ret;
> +
> +	if (auto_exp || !ctrls->exposure->is_new)
> +		return 0;
> +
> +	exp = (u32)ctrls->exposure->val;
> +	exp <<= 4;
> +
> +	return ov2680_write_reg24(sensor, OV2680_REG_EXPOSURE_PK_HIGH, exp);
> +}
> +
> +static int ov2680_exposure_get(struct ov2680_dev *sensor)
> +{
> +	int ret;
> +	u32 exp;
> +
> +	ret = ov2680_read_reg24(sensor, OV2680_REG_EXPOSURE_PK_HIGH, &exp);
> +	if (ret)
> +		return ret;
> +
> +	return exp >> 4;
> +}
> +
> +static int ov2680_auto_exposure_enable(struct ov2680_dev *sensor)
> +{
> +	return ov2680_exposure_set(sensor, true);
> +}
> +
> +static int ov2680_auto_exposure_disable(struct ov2680_dev *sensor)
> +{
> +	return ov2680_exposure_set(sensor, false);
> +}

I still think you could call the function actually doing the work, and pass
the bool parameter. That'd be much clearer. Please see the comments below,
too; they're related. Same for gain.

...

> +static int ov2680_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> +	struct ov2680_dev *sensor = to_ov2680_dev(sd);
> +	int val;
> +
> +	if (!sensor->is_enabled)
> +		return 0;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_AUTOGAIN:
> +		if (!ctrl->val)
> +			return 0;
> +		val = ov2680_gain_get(sensor);
> +		if (val < 0)
> +			return val;
> +		sensor->ctrls.gain->val = val;
> +		break;
> +	case V4L2_CID_EXPOSURE_AUTO:

I reckon the purpose of implementing volatile controls here would be to
provide the exposure and gain values to the user. But the controls here are
the ones enabling or disabling the automatic behaviour, not the value of
those settings themselves.

> +		if (ctrl->val == V4L2_EXPOSURE_MANUAL)
> +			return 0;
> +		val = ov2680_exposure_get(sensor);
> +		if (val < 0)
> +			return val;
> +		sensor->ctrls.exposure->val = val;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
> +	struct ov2680_dev *sensor = to_ov2680_dev(sd);
> +
> +	if (!sensor->is_enabled)
> +		return 0;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_AUTOGAIN:
> +		return ov2680_gain_set(sensor, !!ctrl->val);
> +	case V4L2_CID_EXPOSURE_AUTO:
> +		return ov2680_exposure_set(sensor, !!ctrl->val);

With this, you can enable or disable automatic exposure and gain, but you
cannot manually set the values. Are such controls useful?

Unless I'm mistaken, exposure or gain are not settable, so you should make
them read-only controls. Or better, allow setting them if automatic control
is disabled.

> +	case V4L2_CID_VFLIP:
> +		if (sensor->is_streaming)
> +			return -EBUSY;
> +		if (ctrl->val)
> +			return ov2680_vflip_enable(sensor);
> +		else
> +			return ov2680_vflip_disable(sensor);
> +	case V4L2_CID_HFLIP:
> +		if (ctrl->val)
> +			return ov2680_hflip_enable(sensor);
> +		else
> +			return ov2680_hflip_disable(sensor);
> +	case V4L2_CID_TEST_PATTERN:
> +		return ov2680_test_pattern_set(sensor, ctrl->val);
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct v4l2_ctrl_ops ov2680_ctrl_ops = {
> +	.g_volatile_ctrl = ov2680_g_volatile_ctrl,
> +	.s_ctrl = ov2680_s_ctrl,
> +};
> +
> +static const struct v4l2_subdev_core_ops ov2680_core_ops = {
> +	.s_power = ov2680_s_power,
> +};
> +
> +static const struct v4l2_subdev_video_ops ov2680_video_ops = {
> +	.g_frame_interval	= ov2680_s_g_frame_interval,
> +	.s_frame_interval	= ov2680_s_g_frame_interval,
> +	.s_stream		= ov2680_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops ov2680_pad_ops = {
> +	.enum_mbus_code		= ov2680_enum_mbus_code,
> +	.get_fmt		= ov2680_get_fmt,
> +	.set_fmt		= ov2680_set_fmt,
> +	.enum_frame_size	= ov2680_enum_frame_size,
> +	.enum_frame_interval	= ov2680_enum_frame_interval,
> +};
> +
> +static const struct v4l2_subdev_ops ov2680_subdev_ops = {
> +	.core	= &ov2680_core_ops,
> +	.video	= &ov2680_video_ops,
> +	.pad	= &ov2680_pad_ops,
> +};
> +
> +static int ov2680_mode_init(struct ov2680_dev *sensor)
> +{
> +	const struct ov2680_mode_info *init_mode;
> +
> +	/* set initial mode */
> +	sensor->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +	sensor->fmt.width = 800;
> +	sensor->fmt.height = 600;
> +	sensor->fmt.field = V4L2_FIELD_NONE;
> +	sensor->fmt.colorspace = V4L2_COLORSPACE_SRGB;
> +
> +	sensor->frame_interval.denominator = OV2680_FRAME_RATE;
> +	sensor->frame_interval.numerator = 1;
> +
> +	init_mode = &ov2680_mode_init_data;
> +
> +	sensor->current_mode = init_mode;
> +
> +	sensor->mode_pending_changes = true;
> +
> +	return 0;
> +}
> +
> +static int ov2680_v4l2_init(struct ov2680_dev *sensor)
> +{
> +	const struct v4l2_ctrl_ops *ops = &ov2680_ctrl_ops;
> +	struct ov2680_ctrls *ctrls = &sensor->ctrls;
> +	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> +	int ret = 0;
> +
> +	v4l2_i2c_subdev_init(&sensor->sd, sensor->i2c_client,
> +			     &ov2680_subdev_ops);
> +
> +	sensor->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +
> +	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> +	if (ret < 0)
> +		return ret;
> +
> +	v4l2_ctrl_handler_init(hdl, 32);

You have only 7 controls.

> +
> +	hdl->lock = &sensor->lock;
> +
> +	ctrls->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> +	ctrls->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> +
> +	ctrls->test_pattern = v4l2_ctrl_new_std_menu_items(hdl,
> +					&ov2680_ctrl_ops,
> +					V4L2_CID_TEST_PATTERN,
> +					ARRAY_SIZE(test_pattern_menu) - 1,
> +					0, 0, test_pattern_menu);
> +
> +	ctrls->auto_exp = v4l2_ctrl_new_std_menu(hdl, ops,
> +						 V4L2_CID_EXPOSURE_AUTO,
> +						 V4L2_EXPOSURE_MANUAL, 0,
> +						 V4L2_EXPOSURE_AUTO);
> +
> +	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> +					    0, 32767, 1, 0);
> +
> +	ctrls->auto_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_AUTOGAIN,
> +					     0, 1, 1, 1);
> +	ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 2047, 1, 0);
> +
> +	if (hdl->error) {
> +		ret = hdl->error;
> +		goto cleanup_entity;
> +	}
> +
> +	ctrls->gain->flags |= V4L2_CTRL_FLAG_VOLATILE;
> +	ctrls->exposure->flags |= V4L2_CTRL_FLAG_VOLATILE;
> +
> +	v4l2_ctrl_auto_cluster(2, &ctrls->auto_gain, 0, true);
> +	v4l2_ctrl_auto_cluster(2, &ctrls->auto_exp, 1, true);
> +
> +	sensor->sd.ctrl_handler = hdl;
> +
> +	ret = v4l2_async_register_subdev(&sensor->sd);
> +	if (ret < 0)
> +		goto cleanup_entity;
> +
> +	return 0;
> +
> +cleanup_entity:
> +	media_entity_cleanup(&sensor->sd.entity);
> +	v4l2_ctrl_handler_free(hdl);
> +
> +	return ret;
> +}
> +
> +static int ov2680_check_id(struct ov2680_dev *sensor)
> +{
> +	struct device *dev = ov2680_to_dev(sensor);
> +	u32 chip_id;
> +	int ret;
> +
> +	ov2680_power_on(sensor);
> +
> +	ret = ov2680_read_reg16(sensor, OV2680_REG_CHIP_ID_HIGH, &chip_id);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to read chip id high\n");
> +		return -ENODEV;
> +	}
> +
> +	if (chip_id != OV2680_CHIP_ID) {
> +		dev_err(dev, "chip id: 0x%04x does not match expected 0x%04x\n",
> +			chip_id, OV2680_CHIP_ID);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ov2860_parse_dt(struct ov2680_dev *sensor)
> +{
> +	struct device *dev = ov2680_to_dev(sensor);
> +	int ret;
> +
> +	sensor->pwdn_gpio = devm_gpiod_get_optional(dev, "powerdown",
> +						    GPIOD_OUT_HIGH);
> +	ret = PTR_ERR_OR_ZERO(sensor->pwdn_gpio);
> +	if (ret < 0) {
> +		dev_dbg(dev, "error while getting powerdown gpio: %d\n", ret);
> +		return ret;
> +	}
> +
> +	sensor->xvclk = devm_clk_get(dev, "xvclk");
> +	if (IS_ERR(sensor->xvclk)) {
> +		dev_err(dev, "xvclk clock missing or invalid\n");
> +		return PTR_ERR(sensor->xvclk);
> +	}
> +
> +	sensor->xvclk_freq = clk_get_rate(sensor->xvclk);
> +	if (sensor->xvclk_freq < OV2680_XVCLK_MIN ||
> +	    sensor->xvclk_freq > OV2680_XVCLK_MAX) {

What's the frequency the register configuration requires? I think you
should check the exact frequency here, rather than a wide range. The
register lists would need to be adapted for other frequencies.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@....fi

Powered by blists - more mailing lists