[v3,0/2] media: Introduce Omnivision OV2680 driver

Add driver and bindings for the OV2680 2 megapixel CMOS 1/5" sensor, which has
a single MIPI lane interface and output format of 10-bit Raw RGB.

Features supported are described in PATCH 2/2.

v2->v3:
Rob Herring:
    - add Reviewed-by tag to dts PATCH 1/1

Sakari Ailus:
    - align register values with bracket
    - redone the {write|read}_reg i2c functions
    - add bayer order handling with flip and mirror controls
    - fix error path in probe release resources
    - remove i2c_device_id and use probe_new

Myself:
    - remove ; at the end of macros
    
v1->v2:
Fabio Estevam:
    - s/OV5640/OV2680 in PATCH 1/2 changelog

Sakari Ailus:
    - add description on endpoint properties in bindings
    - add single endpoint in bindings
    - drop OF dependency
    - cleanup includes
    - fix case in Color Bars
    - remove frame rate selection
    - 8/16/24 bit register access in the same transaction
    - merge _reset and _soft_reset to _enable and rename it to power_on 
    - _gain_set use only the gain value (drop & 0x7ff)
    - _gain_get remove the (0x377)
    - single write/read at _exposure_set/get use write_reg24/read_reg24
    - move mode_set_direct to _mode_set
    - _mode_set set auto exposure/gain based on ctrl value
    - s_frame_interval equal to g_frame_interval
    - use closest match from: v4l: common: Add a function to obtain best size from a list 
    - check v4l2_ctrl_new_std return in _init

    - fix gain manual value in auto_cluster

Cheers,
    Rui

Rui Miguel Silva (2):
  media: ov2680: dt: Add bindings for OV2680
  media: ov2680: Add Omnivision OV2680 sensor driver

 .../devicetree/bindings/media/i2c/ov2680.txt       |   40 +
 drivers/media/i2c/Kconfig                          |   12 +
 drivers/media/i2c/Makefile                         |    1 +
 drivers/media/i2c/ov2680.c                         | 1130 ++++++++++++++++++++
 4 files changed, 1183 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2680.txt
 create mode 100644 drivers/media/i2c/ov2680.c

-- 
2.16.2

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@iki.fi