Re: [PATCH v3 22/24] media: imx: Add MIPI CSI-2 OV5640 sensor subdev driver

[Steve Longerbeam]

On 01/20/2017 06:48 AM, Hans Verkuil wrote:
> On 01/07/2017 03:11 AM, Steve Longerbeam wrote:
> > +
> > +	/* cached control settings */
> > +	int ctrl_cache[OV5640_MAX_CONTROLS];
> This is just duplicating the cached value in the control framework. I think this can be dropped.

done, see below.

> > +
> > +static struct ov5640_control ov5640_ctrls[] = {
> > +	{
> > +		.set = ov5640_set_agc,
> > +		.ctrl = {
> > +			.id = V4L2_CID_AUTOGAIN,
> > +			.name = "Auto Gain/Exposure Control",
> > +			.minimum = 0,
> > +			.maximum = 1,
> > +			.step = 1,
> > +			.default_value = 1,
> > +			.type = V4L2_CTRL_TYPE_BOOLEAN,
> > +		},
> > +	}, {
> > +		.set = ov5640_set_exposure,
> > +		.ctrl = {
> > +			.id = V4L2_CID_EXPOSURE,
> > +			.name = "Exposure",
> > +			.minimum = 0,
> > +			.maximum = 65535,
> > +			.step = 1,
> > +			.default_value = 0,
> > +			.type = V4L2_CTRL_TYPE_INTEGER,
> > +		},
> > +	}, {
> > +		.set = ov5640_set_gain,
> > +		.ctrl = {
> > +			.id = V4L2_CID_GAIN,
> > +			.name = "Gain",
> > +			.minimum = 0,
> > +			.maximum = 1023,
> > +			.step = 1,
> > +			.default_value = 0,
> > +			.type = V4L2_CTRL_TYPE_INTEGER,
> > +		},
> > +	}, {
> > +		.set = ov5640_set_hue,
> > +		.ctrl = {
> > +			.id = V4L2_CID_HUE,
> > +			.name = "Hue",
> > +			.minimum = 0,
> > +			.maximum = 359,
> > +			.step = 1,
> > +			.default_value = 0,
> > +			.type = V4L2_CTRL_TYPE_INTEGER,
> > +		},
> > +	}, {
> > +		.set = ov5640_set_contrast,
> > +		.ctrl = {
> > +			.id = V4L2_CID_CONTRAST,
> > +			.name = "Contrast",
> > +			.minimum = 0,
> > +			.maximum = 255,
> > +			.step = 1,
> > +			.default_value = 0,
> > +			.type = V4L2_CTRL_TYPE_INTEGER,
> > +		},
> > +	}, {
> > +		.set = ov5640_set_saturation,
> > +		.ctrl = {
> > +			.id = V4L2_CID_SATURATION,
> > +			.name = "Saturation",
> > +			.minimum = 0,
> > +			.maximum = 255,
> > +			.step = 1,
> > +			.default_value = 64,
> > +			.type = V4L2_CTRL_TYPE_INTEGER,
> > +		},
> > +	}, {
> > +		.set = ov5640_set_awb,
> > +		.ctrl = {
> > +			.id = V4L2_CID_AUTO_WHITE_BALANCE,
> > +			.name = "Auto White Balance",
> > +			.minimum = 0,
> > +			.maximum = 1,
> > +			.step = 1,
> > +			.default_value = 1,
> > +			.type = V4L2_CTRL_TYPE_BOOLEAN,
> > +		},
> > +	}, {
> > +		.set = ov5640_set_red_balance,
> > +		.ctrl = {
> > +			.id = V4L2_CID_RED_BALANCE,
> > +			.name = "Red Balance",
> > +			.minimum = 0,
> > +			.maximum = 4095,
> > +			.step = 1,
> > +			.default_value = 0,
> > +			.type = V4L2_CTRL_TYPE_INTEGER,
> > +		},
> > +	}, {
> > +		.set = ov5640_set_blue_balance,
> > +		.ctrl = {
> > +			.id = V4L2_CID_BLUE_BALANCE,
> > +			.name = "Blue Balance",
> > +			.minimum = 0,
> > +			.maximum = 4095,
> > +			.step = 1,
> > +			.default_value = 0,
> > +			.type = V4L2_CTRL_TYPE_INTEGER,
> > +		},
> > +	},
> > +};
> > +#define OV5640_NUM_CONTROLS ARRAY_SIZE(ov5640_ctrls)
> This should use v4l2_ctrl_new_std() instead of this array.
> Just put a switch on ctrl->id in s_ctrl, and each case calls the corresponding
> set function.

In this case, because there are lots of controls, my preference is to use
table lookup rather than a large switch statement. However I did remove
.name and .type from the table entries, leaving only .def, .min, .max, .step
as required to pass to v4l2_ctrl_new_std(). Also converted to 'struct 
v4l2_ctrl_config'
in the table.


> > +
> > +static int ov5640_restore_ctrls(struct ov5640_dev *sensor)
> > +{
> > +	struct ov5640_control *c;
> > +	int i;
> > +
> > +	for (i = 0; i < OV5640_NUM_CONTROLS; i++) {
> > +		c = &ov5640_ctrls[i];
> > +		c->set(sensor, sensor->ctrl_cache[i]);
> > +	}
> > +
> > +	return 0;
> > +}
> This does the same as v4l2_ctrl_handler_setup() if I understand the code correctly.

yes thanks. I remember looking at this and thinking 
v4l2_ctrl_handler_setup()
was setting up the default values rather than the current values, but 
after a
re-read it does look to be restoring the current values, which is 
exactly what
is needed here.

> > +
> > +static int ov5640_init_controls(struct ov5640_dev *sensor)
> > +{
> > +	struct ov5640_control *c;
> > +	int i;
> > +
> > +	v4l2_ctrl_handler_init(&sensor->ctrl_hdl, OV5640_NUM_CONTROLS);
> > +
> > +	for (i = 0; i < OV5640_NUM_CONTROLS; i++) {
> > +		c = &ov5640_ctrls[i];
> > +
> > +		v4l2_ctrl_new_std(&sensor->ctrl_hdl, &ov5640_ctrl_ops,
> > +				  c->ctrl.id, c->ctrl.minimum, c->ctrl.maximum,
> > +				  c->ctrl.step, c->ctrl.default_value);
> > +	}
> As mentioned, just drop the ov5640_ctrls array and call v4l2_ctr_new_std for each
> control you're adding.

if really pressed I can be persuaded to use a switch statement and call
v4l2_ctrl_new_std() multiple times, but I don't any problem with using
a table.

> > +
> > +module_i2c_driver(ov5640_i2c_driver);
> > +
> > +MODULE_AUTHOR("Freescale Semiconductor, Inc.");
> > +MODULE_AUTHOR("Steve Longerbeam <steve_longerbeam@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("OV5640 MIPI Camera Subdev Driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_VERSION("1.0");
> Same comments apply to the next patch, so I won't repeat them.

done, I've made the same mods to the ov5642 subdev.

Steve