[v2,1/4] ov7670: use the control framework

  Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
Changes since v1:
 - Use v4l2_ctrl_auto_cluster() for auto_gain and auto_exp.

---
 drivers/media/i2c/ov7670.c |  310 ++++++++++++++++----------------------------
 1 file changed, 112 insertions(+), 198 deletions(-)
  

On Fri September 28 2012 12:50:55 Javier Martin wrote:
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> ---
> Changes since v1:
>  - Use v4l2_ctrl_auto_cluster() for auto_gain and auto_exp.
> 
> ---
>  drivers/media/i2c/ov7670.c |  310 ++++++++++++++++----------------------------
>  1 file changed, 112 insertions(+), 198 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index 9d7a92e..babd55c 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -18,6 +18,7 @@
>  #include <linux/videodev2.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-chip-ident.h>
> +#include <media/v4l2-ctrls.h>
>  #include <media/v4l2-mediabus.h>
>  #include <media/ov7670.h>
>  
> @@ -222,9 +223,23 @@ struct ov7670_devtype {
>  struct ov7670_format_struct;  /* coming later */
>  struct ov7670_info {
>  	struct v4l2_subdev sd;
> +	struct v4l2_ctrl_handler hdl;
> +	struct {
> +		/* gain cluster */
> +		struct v4l2_ctrl *auto_gain;
> +		struct v4l2_ctrl *gain;
> +	};
> +	struct {
> +		/* exposure cluster */
> +		struct v4l2_ctrl *auto_exposure;
> +		struct v4l2_ctrl *exposure;
> +	};
> +	struct {
> +		/* saturation/hue cluster */
> +		struct v4l2_ctrl *saturation;
> +		struct v4l2_ctrl *hue;
> +	};
>  	struct ov7670_format_struct *fmt;  /* Current format */
> -	unsigned char sat;		/* Saturation value */
> -	int hue;			/* Hue value */
>  	int min_width;			/* Filter out smaller sizes */
>  	int min_height;			/* Filter out smaller sizes */
>  	int clock_speed;		/* External clock speed (MHz) */
> @@ -240,6 +255,11 @@ static inline struct ov7670_info *to_state(struct v4l2_subdev *sd)
>  	return container_of(sd, struct ov7670_info, sd);
>  }
>  
> +static inline struct v4l2_subdev *to_sd(struct v4l2_ctrl *ctrl)
> +{
> +	return &container_of(ctrl->handler, struct ov7670_info, hdl)->sd;
> +}
> +
>  
>  
>  /*
> @@ -1195,23 +1215,23 @@ static int ov7670_cosine(int theta)
>  
>  
>  static void ov7670_calc_cmatrix(struct ov7670_info *info,
> -		int matrix[CMATRIX_LEN])
> +		int matrix[CMATRIX_LEN], int sat, int hue)
>  {
>  	int i;
>  	/*
>  	 * Apply the current saturation setting first.
>  	 */
>  	for (i = 0; i < CMATRIX_LEN; i++)
> -		matrix[i] = (info->fmt->cmatrix[i]*info->sat) >> 7;
> +		matrix[i] = (info->fmt->cmatrix[i] * sat) >> 7;
>  	/*
>  	 * Then, if need be, rotate the hue value.
>  	 */
> -	if (info->hue != 0) {
> +	if (hue != 0) {
>  		int sinth, costh, tmpmatrix[CMATRIX_LEN];
>  
>  		memcpy(tmpmatrix, matrix, CMATRIX_LEN*sizeof(int));
> -		sinth = ov7670_sine(info->hue);
> -		costh = ov7670_cosine(info->hue);
> +		sinth = ov7670_sine(hue);
> +		costh = ov7670_cosine(hue);
>  
>  		matrix[0] = (matrix[3]*sinth + matrix[0]*costh)/1000;
>  		matrix[1] = (matrix[4]*sinth + matrix[1]*costh)/1000;
> @@ -1224,60 +1244,21 @@ static void ov7670_calc_cmatrix(struct ov7670_info *info,
>  
>  
>  
> -static int ov7670_s_sat(struct v4l2_subdev *sd, int value)
> +static int ov7670_s_sat_hue(struct v4l2_subdev *sd, int sat, int hue)
>  {
>  	struct ov7670_info *info = to_state(sd);
>  	int matrix[CMATRIX_LEN];
>  	int ret;
>  
> -	info->sat = value;
> -	ov7670_calc_cmatrix(info, matrix);
> +	ov7670_calc_cmatrix(info, matrix, sat, hue);
>  	ret = ov7670_store_cmatrix(sd, matrix);
>  	return ret;
>  }
>  
> -static int ov7670_g_sat(struct v4l2_subdev *sd, __s32 *value)
> -{
> -	struct ov7670_info *info = to_state(sd);
> -
> -	*value = info->sat;
> -	return 0;
> -}
> -
> -static int ov7670_s_hue(struct v4l2_subdev *sd, int value)
> -{
> -	struct ov7670_info *info = to_state(sd);
> -	int matrix[CMATRIX_LEN];
> -	int ret;
> -
> -	if (value < -180 || value > 180)
> -		return -EINVAL;
> -	info->hue = value;
> -	ov7670_calc_cmatrix(info, matrix);
> -	ret = ov7670_store_cmatrix(sd, matrix);
> -	return ret;
> -}
> -
> -
> -static int ov7670_g_hue(struct v4l2_subdev *sd, __s32 *value)
> -{
> -	struct ov7670_info *info = to_state(sd);
> -
> -	*value = info->hue;
> -	return 0;
> -}
> -
>  
>  /*
>   * Some weird registers seem to store values in a sign/magnitude format!
>   */
> -static unsigned char ov7670_sm_to_abs(unsigned char v)
> -{
> -	if ((v & 0x80) == 0)
> -		return v + 128;
> -	return 128 - (v & 0x7f);
> -}
> -
>  
>  static unsigned char ov7670_abs_to_sm(unsigned char v)
>  {
> @@ -1299,40 +1280,11 @@ static int ov7670_s_brightness(struct v4l2_subdev *sd, int value)
>  	return ret;
>  }
>  
> -static int ov7670_g_brightness(struct v4l2_subdev *sd, __s32 *value)
> -{
> -	unsigned char v = 0;
> -	int ret = ov7670_read(sd, REG_BRIGHT, &v);
> -
> -	*value = ov7670_sm_to_abs(v);
> -	return ret;
> -}
> -
>  static int ov7670_s_contrast(struct v4l2_subdev *sd, int value)
>  {
>  	return ov7670_write(sd, REG_CONTRAS, (unsigned char) value);
>  }
>  
> -static int ov7670_g_contrast(struct v4l2_subdev *sd, __s32 *value)
> -{
> -	unsigned char v = 0;
> -	int ret = ov7670_read(sd, REG_CONTRAS, &v);
> -
> -	*value = v;
> -	return ret;
> -}
> -
> -static int ov7670_g_hflip(struct v4l2_subdev *sd, __s32 *value)
> -{
> -	int ret;
> -	unsigned char v = 0;
> -
> -	ret = ov7670_read(sd, REG_MVFP, &v);
> -	*value = (v & MVFP_MIRROR) == MVFP_MIRROR;
> -	return ret;
> -}
> -
> -
>  static int ov7670_s_hflip(struct v4l2_subdev *sd, int value)
>  {
>  	unsigned char v = 0;
> @@ -1348,19 +1300,6 @@ static int ov7670_s_hflip(struct v4l2_subdev *sd, int value)
>  	return ret;
>  }
>  
> -
> -
> -static int ov7670_g_vflip(struct v4l2_subdev *sd, __s32 *value)
> -{
> -	int ret;
> -	unsigned char v = 0;
> -
> -	ret = ov7670_read(sd, REG_MVFP, &v);
> -	*value = (v & MVFP_FLIP) == MVFP_FLIP;
> -	return ret;
> -}
> -
> -
>  static int ov7670_s_vflip(struct v4l2_subdev *sd, int value)
>  {
>  	unsigned char v = 0;
> @@ -1409,16 +1348,6 @@ static int ov7670_s_gain(struct v4l2_subdev *sd, int value)
>  /*
>   * Tweak autogain.
>   */
> -static int ov7670_g_autogain(struct v4l2_subdev *sd, __s32 *value)
> -{
> -	int ret;
> -	unsigned char com8;
> -
> -	ret = ov7670_read(sd, REG_COM8, &com8);
> -	*value = (com8 & COM8_AGC) != 0;
> -	return ret;
> -}
> -
>  static int ov7670_s_autogain(struct v4l2_subdev *sd, int value)
>  {
>  	int ret;
> @@ -1435,22 +1364,6 @@ static int ov7670_s_autogain(struct v4l2_subdev *sd, int value)
>  	return ret;
>  }
>  
> -/*
> - * Exposure is spread all over the place: top 6 bits in AECHH, middle
> - * 8 in AECH, and two stashed in COM1 just for the hell of it.
> - */
> -static int ov7670_g_exp(struct v4l2_subdev *sd, __s32 *value)
> -{
> -	int ret;
> -	unsigned char com1, aech, aechh;
> -
> -	ret = ov7670_read(sd, REG_COM1, &com1) +
> -		ov7670_read(sd, REG_AECH, &aech) +
> -		ov7670_read(sd, REG_AECHH, &aechh);
> -	*value = ((aechh & 0x3f) << 10) | (aech << 2) | (com1 & 0x03);
> -	return ret;
> -}
> -
>  static int ov7670_s_exp(struct v4l2_subdev *sd, int value)
>  {
>  	int ret;
> @@ -1477,20 +1390,6 @@ static int ov7670_s_exp(struct v4l2_subdev *sd, int value)
>  /*
>   * Tweak autoexposure.
>   */
> -static int ov7670_g_autoexp(struct v4l2_subdev *sd, __s32 *value)
> -{
> -	int ret;
> -	unsigned char com8;
> -	enum v4l2_exposure_auto_type *atype = (enum v4l2_exposure_auto_type *) value;
> -
> -	ret = ov7670_read(sd, REG_COM8, &com8);
> -	if (com8 & COM8_AEC)
> -		*atype = V4L2_EXPOSURE_AUTO;
> -	else
> -		*atype = V4L2_EXPOSURE_MANUAL;
> -	return ret;
> -}
> -
>  static int ov7670_s_autoexp(struct v4l2_subdev *sd,
>  		enum v4l2_exposure_auto_type value)
>  {
> @@ -1509,90 +1408,60 @@ static int ov7670_s_autoexp(struct v4l2_subdev *sd,
>  }
>  
>  
> -
> -static int ov7670_queryctrl(struct v4l2_subdev *sd,
> -		struct v4l2_queryctrl *qc)
> +static int ov7670_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>  {
> -	/* Fill in min, max, step and default value for these controls. */
> -	switch (qc->id) {
> -	case V4L2_CID_BRIGHTNESS:
> -		return v4l2_ctrl_query_fill(qc, 0, 255, 1, 128);
> -	case V4L2_CID_CONTRAST:
> -		return v4l2_ctrl_query_fill(qc, 0, 127, 1, 64);
> -	case V4L2_CID_VFLIP:
> -	case V4L2_CID_HFLIP:
> -		return v4l2_ctrl_query_fill(qc, 0, 1, 1, 0);
> -	case V4L2_CID_SATURATION:
> -		return v4l2_ctrl_query_fill(qc, 0, 256, 1, 128);
> -	case V4L2_CID_HUE:
> -		return v4l2_ctrl_query_fill(qc, -180, 180, 5, 0);
> -	case V4L2_CID_GAIN:
> -		return v4l2_ctrl_query_fill(qc, 0, 255, 1, 128);
> -	case V4L2_CID_AUTOGAIN:
> -		return v4l2_ctrl_query_fill(qc, 0, 1, 1, 1);
> -	case V4L2_CID_EXPOSURE:
> -		return v4l2_ctrl_query_fill(qc, 0, 65535, 1, 500);
> -	case V4L2_CID_EXPOSURE_AUTO:
> -		return v4l2_ctrl_query_fill(qc, 0, 1, 1, 0);
> -	}
> -	return -EINVAL;
> -}
> +	struct v4l2_subdev *sd = to_sd(ctrl);
> +	struct ov7670_info *info = to_state(sd);
>  
> -static int ov7670_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> -{
>  	switch (ctrl->id) {
> -	case V4L2_CID_BRIGHTNESS:
> -		return ov7670_g_brightness(sd, &ctrl->value);
> -	case V4L2_CID_CONTRAST:
> -		return ov7670_g_contrast(sd, &ctrl->value);
> -	case V4L2_CID_SATURATION:
> -		return ov7670_g_sat(sd, &ctrl->value);
> -	case V4L2_CID_HUE:
> -		return ov7670_g_hue(sd, &ctrl->value);
> -	case V4L2_CID_VFLIP:
> -		return ov7670_g_vflip(sd, &ctrl->value);
> -	case V4L2_CID_HFLIP:
> -		return ov7670_g_hflip(sd, &ctrl->value);
> -	case V4L2_CID_GAIN:
> -		return ov7670_g_gain(sd, &ctrl->value);
>  	case V4L2_CID_AUTOGAIN:
> -		return ov7670_g_autogain(sd, &ctrl->value);
> -	case V4L2_CID_EXPOSURE:
> -		return ov7670_g_exp(sd, &ctrl->value);
> -	case V4L2_CID_EXPOSURE_AUTO:
> -		return ov7670_g_autoexp(sd, &ctrl->value);
> +		return ov7670_g_gain(sd, &info->gain->cur.val);

This should be &info->gain->val, not cur.val. It used to be cur.val in the
past, but that was changed. You never change cur.val directly.

Out of curiosity: what made you decide to use cur.val here? I'm wondering if
I should improve the documentation if it is confusing people.

>  	}
>  	return -EINVAL;
>  }
>  
> -static int ov7670_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
> +static int ov7670_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
> +	struct v4l2_subdev *sd = to_sd(ctrl);
> +	struct ov7670_info *info = to_state(sd);
> +
>  	switch (ctrl->id) {
>  	case V4L2_CID_BRIGHTNESS:
> -		return ov7670_s_brightness(sd, ctrl->value);
> +		return ov7670_s_brightness(sd, ctrl->val);
>  	case V4L2_CID_CONTRAST:
> -		return ov7670_s_contrast(sd, ctrl->value);
> +		return ov7670_s_contrast(sd, ctrl->val);
>  	case V4L2_CID_SATURATION:
> -		return ov7670_s_sat(sd, ctrl->value);
> -	case V4L2_CID_HUE:
> -		return ov7670_s_hue(sd, ctrl->value);
> +		return ov7670_s_sat_hue(sd,
> +				info->saturation->val, info->hue->val);
>  	case V4L2_CID_VFLIP:
> -		return ov7670_s_vflip(sd, ctrl->value);
> +		return ov7670_s_vflip(sd, ctrl->val);
>  	case V4L2_CID_HFLIP:
> -		return ov7670_s_hflip(sd, ctrl->value);
> -	case V4L2_CID_GAIN:
> -		return ov7670_s_gain(sd, ctrl->value);
> +		return ov7670_s_hflip(sd, ctrl->val);
>  	case V4L2_CID_AUTOGAIN:
> -		return ov7670_s_autogain(sd, ctrl->value);
> -	case V4L2_CID_EXPOSURE:
> -		return ov7670_s_exp(sd, ctrl->value);
> +		/* Only set manual gain if auto gain is not explicitly
> +		   turned on. */
> +		if (!ctrl->val) {
> +			/* ov7670_s_gain turns off auto gain */
> +			return ov7670_s_gain(sd, info->gain->val);
> +		}
> +		return ov7670_s_autogain(sd, ctrl->val);
>  	case V4L2_CID_EXPOSURE_AUTO:
> -		return ov7670_s_autoexp(sd,
> -				(enum v4l2_exposure_auto_type) ctrl->value);
> +		/* Only set manual exposure if auto exposure is not explicitly
> +		   turned on. */
> +		if (ctrl->val == V4L2_EXPOSURE_MANUAL) {
> +			/* ov7670_s_exp turns off auto exposure */
> +			return ov7670_s_exp(sd, info->exposure->val);
> +		}
> +		return ov7670_s_autoexp(sd, ctrl->val);
>  	}
>  	return -EINVAL;
>  }
>  
> +static const struct v4l2_ctrl_ops ov7670_ctrl_ops = {
> +	.s_ctrl = ov7670_s_ctrl,
> +	.g_volatile_ctrl = ov7670_g_volatile_ctrl,
> +};
> +
>  static int ov7670_g_chip_ident(struct v4l2_subdev *sd,
>  		struct v4l2_dbg_chip_ident *chip)
>  {
> @@ -1635,9 +1504,13 @@ static int ov7670_s_register(struct v4l2_subdev *sd, struct v4l2_dbg_register *r
>  
>  static const struct v4l2_subdev_core_ops ov7670_core_ops = {
>  	.g_chip_ident = ov7670_g_chip_ident,
> -	.g_ctrl = ov7670_g_ctrl,
> -	.s_ctrl = ov7670_s_ctrl,
> -	.queryctrl = ov7670_queryctrl,
> +	.g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
> +	.try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
> +	.s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
> +	.g_ctrl = v4l2_subdev_g_ctrl,
> +	.s_ctrl = v4l2_subdev_s_ctrl,
> +	.queryctrl = v4l2_subdev_queryctrl,
> +	.querymenu = v4l2_subdev_querymenu,
>  	.reset = ov7670_reset,
>  	.init = ov7670_init,
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
> @@ -1732,7 +1605,6 @@ static int ov7670_probe(struct i2c_client *client,
>  
>  	info->devtype = &ov7670_devdata[id->driver_data];
>  	info->fmt = &ov7670_formats[0];
> -	info->sat = 128;	/* Review this */
>  	info->clkrc = 0;
>  
>  	/* Set default frame rate to 30 fps */
> @@ -1743,6 +1615,46 @@ static int ov7670_probe(struct i2c_client *client,
>  	if (info->pclk_hb_disable)
>  		ov7670_write(sd, REG_COM10, COM10_PCLK_HB);
>  
> +	v4l2_ctrl_handler_init(&info->hdl, 10);
> +	v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
> +			V4L2_CID_BRIGHTNESS, 0, 255, 1, 128);
> +	v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
> +			V4L2_CID_CONTRAST, 0, 127, 1, 64);
> +	v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
> +			V4L2_CID_VFLIP, 0, 1, 1, 0);
> +	v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
> +			V4L2_CID_HFLIP, 0, 1, 1, 0);
> +	info->saturation = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
> +			V4L2_CID_SATURATION, 0, 256, 1, 128);
> +	info->hue = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
> +			V4L2_CID_HUE, -180, 180, 5, 0);
> +	info->gain = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
> +			V4L2_CID_GAIN, 0, 255, 1, 128);
> +	info->auto_gain = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
> +			V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
> +	info->exposure = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
> +			V4L2_CID_EXPOSURE, 0, 65535, 1, 500);
> +	info->auto_exposure = v4l2_ctrl_new_std_menu(&info->hdl, &ov7670_ctrl_ops,
> +			V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL, 0,
> +			V4L2_EXPOSURE_AUTO);
> +	sd->ctrl_handler = &info->hdl;
> +	if (info->hdl.error) {
> +		int err = info->hdl.error;
> +
> +		v4l2_ctrl_handler_free(&info->hdl);
> +		kfree(info);
> +		return err;
> +	}
> +	/*
> +	 * We have checked empirically that hw allows to read back the gain
> +	 * value chosen by auto gain but that's not the case for auto exposure.
> +	 */
> +	v4l2_ctrl_auto_cluster(2, &info->auto_gain, 0, true);
> +	v4l2_ctrl_auto_cluster(2, &info->auto_exposure,
> +			       V4L2_EXPOSURE_MANUAL, false);
> +	v4l2_ctrl_cluster(2, &info->saturation);
> +	v4l2_ctrl_handler_setup(&info->hdl);
> +
>  	return 0;
>  }
>  
> @@ -1750,9 +1662,11 @@ static int ov7670_probe(struct i2c_client *client,
>  static int ov7670_remove(struct i2c_client *client)
>  {
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov7670_info *info = to_state(sd);
>  
>  	v4l2_device_unregister_subdev(sd);
> -	kfree(to_state(sd));
> +	v4l2_ctrl_handler_free(&info->hdl);
> +	kfree(info);
>  	return 0;
>  }
>  
> 

After you fixed that single problem I found, you can add:

Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>

for the whole patch series.

Thanks!

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Hans,

On 28 September 2012 13:05, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Fri September 28 2012 12:50:55 Javier Martin wrote:
>> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
>> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>> ---
>> Changes since v1:
>>  - Use v4l2_ctrl_auto_cluster() for auto_gain and auto_exp.
>>
>> ---
>>  drivers/media/i2c/ov7670.c |  310 ++++++++++++++++----------------------------
>>  1 file changed, 112 insertions(+), 198 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
>> index 9d7a92e..babd55c 100644
>> --- a/drivers/media/i2c/ov7670.c
>> +++ b/drivers/media/i2c/ov7670.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/videodev2.h>
>>  #include <media/v4l2-device.h>
>>  #include <media/v4l2-chip-ident.h>
>> +#include <media/v4l2-ctrls.h>
>>  #include <media/v4l2-mediabus.h>
>>  #include <media/ov7670.h>
>>
>> @@ -222,9 +223,23 @@ struct ov7670_devtype {
>>  struct ov7670_format_struct;  /* coming later */
>>  struct ov7670_info {
>>       struct v4l2_subdev sd;
>> +     struct v4l2_ctrl_handler hdl;
>> +     struct {
>> +             /* gain cluster */
>> +             struct v4l2_ctrl *auto_gain;
>> +             struct v4l2_ctrl *gain;
>> +     };
>> +     struct {
>> +             /* exposure cluster */
>> +             struct v4l2_ctrl *auto_exposure;
>> +             struct v4l2_ctrl *exposure;
>> +     };
>> +     struct {
>> +             /* saturation/hue cluster */
>> +             struct v4l2_ctrl *saturation;
>> +             struct v4l2_ctrl *hue;
>> +     };
>>       struct ov7670_format_struct *fmt;  /* Current format */
>> -     unsigned char sat;              /* Saturation value */
>> -     int hue;                        /* Hue value */
>>       int min_width;                  /* Filter out smaller sizes */
>>       int min_height;                 /* Filter out smaller sizes */
>>       int clock_speed;                /* External clock speed (MHz) */
>> @@ -240,6 +255,11 @@ static inline struct ov7670_info *to_state(struct v4l2_subdev *sd)
>>       return container_of(sd, struct ov7670_info, sd);
>>  }
>>
>> +static inline struct v4l2_subdev *to_sd(struct v4l2_ctrl *ctrl)
>> +{
>> +     return &container_of(ctrl->handler, struct ov7670_info, hdl)->sd;
>> +}
>> +
>>
>>
>>  /*
>> @@ -1195,23 +1215,23 @@ static int ov7670_cosine(int theta)
>>
>>
>>  static void ov7670_calc_cmatrix(struct ov7670_info *info,
>> -             int matrix[CMATRIX_LEN])
>> +             int matrix[CMATRIX_LEN], int sat, int hue)
>>  {
>>       int i;
>>       /*
>>        * Apply the current saturation setting first.
>>        */
>>       for (i = 0; i < CMATRIX_LEN; i++)
>> -             matrix[i] = (info->fmt->cmatrix[i]*info->sat) >> 7;
>> +             matrix[i] = (info->fmt->cmatrix[i] * sat) >> 7;
>>       /*
>>        * Then, if need be, rotate the hue value.
>>        */
>> -     if (info->hue != 0) {
>> +     if (hue != 0) {
>>               int sinth, costh, tmpmatrix[CMATRIX_LEN];
>>
>>               memcpy(tmpmatrix, matrix, CMATRIX_LEN*sizeof(int));
>> -             sinth = ov7670_sine(info->hue);
>> -             costh = ov7670_cosine(info->hue);
>> +             sinth = ov7670_sine(hue);
>> +             costh = ov7670_cosine(hue);
>>
>>               matrix[0] = (matrix[3]*sinth + matrix[0]*costh)/1000;
>>               matrix[1] = (matrix[4]*sinth + matrix[1]*costh)/1000;
>> @@ -1224,60 +1244,21 @@ static void ov7670_calc_cmatrix(struct ov7670_info *info,
>>
>>
>>
>> -static int ov7670_s_sat(struct v4l2_subdev *sd, int value)
>> +static int ov7670_s_sat_hue(struct v4l2_subdev *sd, int sat, int hue)
>>  {
>>       struct ov7670_info *info = to_state(sd);
>>       int matrix[CMATRIX_LEN];
>>       int ret;
>>
>> -     info->sat = value;
>> -     ov7670_calc_cmatrix(info, matrix);
>> +     ov7670_calc_cmatrix(info, matrix, sat, hue);
>>       ret = ov7670_store_cmatrix(sd, matrix);
>>       return ret;
>>  }
>>
>> -static int ov7670_g_sat(struct v4l2_subdev *sd, __s32 *value)
>> -{
>> -     struct ov7670_info *info = to_state(sd);
>> -
>> -     *value = info->sat;
>> -     return 0;
>> -}
>> -
>> -static int ov7670_s_hue(struct v4l2_subdev *sd, int value)
>> -{
>> -     struct ov7670_info *info = to_state(sd);
>> -     int matrix[CMATRIX_LEN];
>> -     int ret;
>> -
>> -     if (value < -180 || value > 180)
>> -             return -EINVAL;
>> -     info->hue = value;
>> -     ov7670_calc_cmatrix(info, matrix);
>> -     ret = ov7670_store_cmatrix(sd, matrix);
>> -     return ret;
>> -}
>> -
>> -
>> -static int ov7670_g_hue(struct v4l2_subdev *sd, __s32 *value)
>> -{
>> -     struct ov7670_info *info = to_state(sd);
>> -
>> -     *value = info->hue;
>> -     return 0;
>> -}
>> -
>>
>>  /*
>>   * Some weird registers seem to store values in a sign/magnitude format!
>>   */
>> -static unsigned char ov7670_sm_to_abs(unsigned char v)
>> -{
>> -     if ((v & 0x80) == 0)
>> -             return v + 128;
>> -     return 128 - (v & 0x7f);
>> -}
>> -
>>
>>  static unsigned char ov7670_abs_to_sm(unsigned char v)
>>  {
>> @@ -1299,40 +1280,11 @@ static int ov7670_s_brightness(struct v4l2_subdev *sd, int value)
>>       return ret;
>>  }
>>
>> -static int ov7670_g_brightness(struct v4l2_subdev *sd, __s32 *value)
>> -{
>> -     unsigned char v = 0;
>> -     int ret = ov7670_read(sd, REG_BRIGHT, &v);
>> -
>> -     *value = ov7670_sm_to_abs(v);
>> -     return ret;
>> -}
>> -
>>  static int ov7670_s_contrast(struct v4l2_subdev *sd, int value)
>>  {
>>       return ov7670_write(sd, REG_CONTRAS, (unsigned char) value);
>>  }
>>
>> -static int ov7670_g_contrast(struct v4l2_subdev *sd, __s32 *value)
>> -{
>> -     unsigned char v = 0;
>> -     int ret = ov7670_read(sd, REG_CONTRAS, &v);
>> -
>> -     *value = v;
>> -     return ret;
>> -}
>> -
>> -static int ov7670_g_hflip(struct v4l2_subdev *sd, __s32 *value)
>> -{
>> -     int ret;
>> -     unsigned char v = 0;
>> -
>> -     ret = ov7670_read(sd, REG_MVFP, &v);
>> -     *value = (v & MVFP_MIRROR) == MVFP_MIRROR;
>> -     return ret;
>> -}
>> -
>> -
>>  static int ov7670_s_hflip(struct v4l2_subdev *sd, int value)
>>  {
>>       unsigned char v = 0;
>> @@ -1348,19 +1300,6 @@ static int ov7670_s_hflip(struct v4l2_subdev *sd, int value)
>>       return ret;
>>  }
>>
>> -
>> -
>> -static int ov7670_g_vflip(struct v4l2_subdev *sd, __s32 *value)
>> -{
>> -     int ret;
>> -     unsigned char v = 0;
>> -
>> -     ret = ov7670_read(sd, REG_MVFP, &v);
>> -     *value = (v & MVFP_FLIP) == MVFP_FLIP;
>> -     return ret;
>> -}
>> -
>> -
>>  static int ov7670_s_vflip(struct v4l2_subdev *sd, int value)
>>  {
>>       unsigned char v = 0;
>> @@ -1409,16 +1348,6 @@ static int ov7670_s_gain(struct v4l2_subdev *sd, int value)
>>  /*
>>   * Tweak autogain.
>>   */
>> -static int ov7670_g_autogain(struct v4l2_subdev *sd, __s32 *value)
>> -{
>> -     int ret;
>> -     unsigned char com8;
>> -
>> -     ret = ov7670_read(sd, REG_COM8, &com8);
>> -     *value = (com8 & COM8_AGC) != 0;
>> -     return ret;
>> -}
>> -
>>  static int ov7670_s_autogain(struct v4l2_subdev *sd, int value)
>>  {
>>       int ret;
>> @@ -1435,22 +1364,6 @@ static int ov7670_s_autogain(struct v4l2_subdev *sd, int value)
>>       return ret;
>>  }
>>
>> -/*
>> - * Exposure is spread all over the place: top 6 bits in AECHH, middle
>> - * 8 in AECH, and two stashed in COM1 just for the hell of it.
>> - */
>> -static int ov7670_g_exp(struct v4l2_subdev *sd, __s32 *value)
>> -{
>> -     int ret;
>> -     unsigned char com1, aech, aechh;
>> -
>> -     ret = ov7670_read(sd, REG_COM1, &com1) +
>> -             ov7670_read(sd, REG_AECH, &aech) +
>> -             ov7670_read(sd, REG_AECHH, &aechh);
>> -     *value = ((aechh & 0x3f) << 10) | (aech << 2) | (com1 & 0x03);
>> -     return ret;
>> -}
>> -
>>  static int ov7670_s_exp(struct v4l2_subdev *sd, int value)
>>  {
>>       int ret;
>> @@ -1477,20 +1390,6 @@ static int ov7670_s_exp(struct v4l2_subdev *sd, int value)
>>  /*
>>   * Tweak autoexposure.
>>   */
>> -static int ov7670_g_autoexp(struct v4l2_subdev *sd, __s32 *value)
>> -{
>> -     int ret;
>> -     unsigned char com8;
>> -     enum v4l2_exposure_auto_type *atype = (enum v4l2_exposure_auto_type *) value;
>> -
>> -     ret = ov7670_read(sd, REG_COM8, &com8);
>> -     if (com8 & COM8_AEC)
>> -             *atype = V4L2_EXPOSURE_AUTO;
>> -     else
>> -             *atype = V4L2_EXPOSURE_MANUAL;
>> -     return ret;
>> -}
>> -
>>  static int ov7670_s_autoexp(struct v4l2_subdev *sd,
>>               enum v4l2_exposure_auto_type value)
>>  {
>> @@ -1509,90 +1408,60 @@ static int ov7670_s_autoexp(struct v4l2_subdev *sd,
>>  }
>>
>>
>> -
>> -static int ov7670_queryctrl(struct v4l2_subdev *sd,
>> -             struct v4l2_queryctrl *qc)
>> +static int ov7670_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>>  {
>> -     /* Fill in min, max, step and default value for these controls. */
>> -     switch (qc->id) {
>> -     case V4L2_CID_BRIGHTNESS:
>> -             return v4l2_ctrl_query_fill(qc, 0, 255, 1, 128);
>> -     case V4L2_CID_CONTRAST:
>> -             return v4l2_ctrl_query_fill(qc, 0, 127, 1, 64);
>> -     case V4L2_CID_VFLIP:
>> -     case V4L2_CID_HFLIP:
>> -             return v4l2_ctrl_query_fill(qc, 0, 1, 1, 0);
>> -     case V4L2_CID_SATURATION:
>> -             return v4l2_ctrl_query_fill(qc, 0, 256, 1, 128);
>> -     case V4L2_CID_HUE:
>> -             return v4l2_ctrl_query_fill(qc, -180, 180, 5, 0);
>> -     case V4L2_CID_GAIN:
>> -             return v4l2_ctrl_query_fill(qc, 0, 255, 1, 128);
>> -     case V4L2_CID_AUTOGAIN:
>> -             return v4l2_ctrl_query_fill(qc, 0, 1, 1, 1);
>> -     case V4L2_CID_EXPOSURE:
>> -             return v4l2_ctrl_query_fill(qc, 0, 65535, 1, 500);
>> -     case V4L2_CID_EXPOSURE_AUTO:
>> -             return v4l2_ctrl_query_fill(qc, 0, 1, 1, 0);
>> -     }
>> -     return -EINVAL;
>> -}
>> +     struct v4l2_subdev *sd = to_sd(ctrl);
>> +     struct ov7670_info *info = to_state(sd);
>>
>> -static int ov7670_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
>> -{
>>       switch (ctrl->id) {
>> -     case V4L2_CID_BRIGHTNESS:
>> -             return ov7670_g_brightness(sd, &ctrl->value);
>> -     case V4L2_CID_CONTRAST:
>> -             return ov7670_g_contrast(sd, &ctrl->value);
>> -     case V4L2_CID_SATURATION:
>> -             return ov7670_g_sat(sd, &ctrl->value);
>> -     case V4L2_CID_HUE:
>> -             return ov7670_g_hue(sd, &ctrl->value);
>> -     case V4L2_CID_VFLIP:
>> -             return ov7670_g_vflip(sd, &ctrl->value);
>> -     case V4L2_CID_HFLIP:
>> -             return ov7670_g_hflip(sd, &ctrl->value);
>> -     case V4L2_CID_GAIN:
>> -             return ov7670_g_gain(sd, &ctrl->value);
>>       case V4L2_CID_AUTOGAIN:
>> -             return ov7670_g_autogain(sd, &ctrl->value);
>> -     case V4L2_CID_EXPOSURE:
>> -             return ov7670_g_exp(sd, &ctrl->value);
>> -     case V4L2_CID_EXPOSURE_AUTO:
>> -             return ov7670_g_autoexp(sd, &ctrl->value);
>> +             return ov7670_g_gain(sd, &info->gain->cur.val);
>
> This should be &info->gain->val, not cur.val. It used to be cur.val in the
> past, but that was changed. You never change cur.val directly.
>
> Out of curiosity: what made you decide to use cur.val here? I'm wondering if
> I should improve the documentation if it is confusing people.

It's just that I wasn't careful enough and forgot rests of the old code.+

>>       }
>>       return -EINVAL;
>>  }
>>
>> -static int ov7670_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
>> +static int ov7670_s_ctrl(struct v4l2_ctrl *ctrl)
>>  {
>> +     struct v4l2_subdev *sd = to_sd(ctrl);
>> +     struct ov7670_info *info = to_state(sd);
>> +
>>       switch (ctrl->id) {
>>       case V4L2_CID_BRIGHTNESS:
>> -             return ov7670_s_brightness(sd, ctrl->value);
>> +             return ov7670_s_brightness(sd, ctrl->val);
>>       case V4L2_CID_CONTRAST:
>> -             return ov7670_s_contrast(sd, ctrl->value);
>> +             return ov7670_s_contrast(sd, ctrl->val);
>>       case V4L2_CID_SATURATION:
>> -             return ov7670_s_sat(sd, ctrl->value);
>> -     case V4L2_CID_HUE:
>> -             return ov7670_s_hue(sd, ctrl->value);
>> +             return ov7670_s_sat_hue(sd,
>> +                             info->saturation->val, info->hue->val);
>>       case V4L2_CID_VFLIP:
>> -             return ov7670_s_vflip(sd, ctrl->value);
>> +             return ov7670_s_vflip(sd, ctrl->val);
>>       case V4L2_CID_HFLIP:
>> -             return ov7670_s_hflip(sd, ctrl->value);
>> -     case V4L2_CID_GAIN:
>> -             return ov7670_s_gain(sd, ctrl->value);
>> +             return ov7670_s_hflip(sd, ctrl->val);
>>       case V4L2_CID_AUTOGAIN:
>> -             return ov7670_s_autogain(sd, ctrl->value);
>> -     case V4L2_CID_EXPOSURE:
>> -             return ov7670_s_exp(sd, ctrl->value);
>> +             /* Only set manual gain if auto gain is not explicitly
>> +                turned on. */
>> +             if (!ctrl->val) {
>> +                     /* ov7670_s_gain turns off auto gain */
>> +                     return ov7670_s_gain(sd, info->gain->val);
>> +             }
>> +             return ov7670_s_autogain(sd, ctrl->val);
>>       case V4L2_CID_EXPOSURE_AUTO:
>> -             return ov7670_s_autoexp(sd,
>> -                             (enum v4l2_exposure_auto_type) ctrl->value);
>> +             /* Only set manual exposure if auto exposure is not explicitly
>> +                turned on. */
>> +             if (ctrl->val == V4L2_EXPOSURE_MANUAL) {
>> +                     /* ov7670_s_exp turns off auto exposure */
>> +                     return ov7670_s_exp(sd, info->exposure->val);
>> +             }
>> +             return ov7670_s_autoexp(sd, ctrl->val);
>>       }
>>       return -EINVAL;
>>  }
>>
>> +static const struct v4l2_ctrl_ops ov7670_ctrl_ops = {
>> +     .s_ctrl = ov7670_s_ctrl,
>> +     .g_volatile_ctrl = ov7670_g_volatile_ctrl,
>> +};
>> +
>>  static int ov7670_g_chip_ident(struct v4l2_subdev *sd,
>>               struct v4l2_dbg_chip_ident *chip)
>>  {
>> @@ -1635,9 +1504,13 @@ static int ov7670_s_register(struct v4l2_subdev *sd, struct v4l2_dbg_register *r
>>
>>  static const struct v4l2_subdev_core_ops ov7670_core_ops = {
>>       .g_chip_ident = ov7670_g_chip_ident,
>> -     .g_ctrl = ov7670_g_ctrl,
>> -     .s_ctrl = ov7670_s_ctrl,
>> -     .queryctrl = ov7670_queryctrl,
>> +     .g_ext_ctrls = v4l2_subdev_g_ext_ctrls,
>> +     .try_ext_ctrls = v4l2_subdev_try_ext_ctrls,
>> +     .s_ext_ctrls = v4l2_subdev_s_ext_ctrls,
>> +     .g_ctrl = v4l2_subdev_g_ctrl,
>> +     .s_ctrl = v4l2_subdev_s_ctrl,
>> +     .queryctrl = v4l2_subdev_queryctrl,
>> +     .querymenu = v4l2_subdev_querymenu,
>>       .reset = ov7670_reset,
>>       .init = ov7670_init,
>>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>> @@ -1732,7 +1605,6 @@ static int ov7670_probe(struct i2c_client *client,
>>
>>       info->devtype = &ov7670_devdata[id->driver_data];
>>       info->fmt = &ov7670_formats[0];
>> -     info->sat = 128;        /* Review this */
>>       info->clkrc = 0;
>>
>>       /* Set default frame rate to 30 fps */
>> @@ -1743,6 +1615,46 @@ static int ov7670_probe(struct i2c_client *client,
>>       if (info->pclk_hb_disable)
>>               ov7670_write(sd, REG_COM10, COM10_PCLK_HB);
>>
>> +     v4l2_ctrl_handler_init(&info->hdl, 10);
>> +     v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
>> +                     V4L2_CID_BRIGHTNESS, 0, 255, 1, 128);
>> +     v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
>> +                     V4L2_CID_CONTRAST, 0, 127, 1, 64);
>> +     v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
>> +                     V4L2_CID_VFLIP, 0, 1, 1, 0);
>> +     v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
>> +                     V4L2_CID_HFLIP, 0, 1, 1, 0);
>> +     info->saturation = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
>> +                     V4L2_CID_SATURATION, 0, 256, 1, 128);
>> +     info->hue = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
>> +                     V4L2_CID_HUE, -180, 180, 5, 0);
>> +     info->gain = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
>> +                     V4L2_CID_GAIN, 0, 255, 1, 128);
>> +     info->auto_gain = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
>> +                     V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
>> +     info->exposure = v4l2_ctrl_new_std(&info->hdl, &ov7670_ctrl_ops,
>> +                     V4L2_CID_EXPOSURE, 0, 65535, 1, 500);
>> +     info->auto_exposure = v4l2_ctrl_new_std_menu(&info->hdl, &ov7670_ctrl_ops,
>> +                     V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL, 0,
>> +                     V4L2_EXPOSURE_AUTO);
>> +     sd->ctrl_handler = &info->hdl;
>> +     if (info->hdl.error) {
>> +             int err = info->hdl.error;
>> +
>> +             v4l2_ctrl_handler_free(&info->hdl);
>> +             kfree(info);
>> +             return err;
>> +     }
>> +     /*
>> +      * We have checked empirically that hw allows to read back the gain
>> +      * value chosen by auto gain but that's not the case for auto exposure.
>> +      */
>> +     v4l2_ctrl_auto_cluster(2, &info->auto_gain, 0, true);
>> +     v4l2_ctrl_auto_cluster(2, &info->auto_exposure,
>> +                            V4L2_EXPOSURE_MANUAL, false);
>> +     v4l2_ctrl_cluster(2, &info->saturation);
>> +     v4l2_ctrl_handler_setup(&info->hdl);
>> +
>>       return 0;
>>  }
>>
>> @@ -1750,9 +1662,11 @@ static int ov7670_probe(struct i2c_client *client,
>>  static int ov7670_remove(struct i2c_client *client)
>>  {
>>       struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> +     struct ov7670_info *info = to_state(sd);
>>
>>       v4l2_device_unregister_subdev(sd);
>> -     kfree(to_state(sd));
>> +     v4l2_ctrl_handler_free(&info->hdl);
>> +     kfree(info);
>>       return 0;
>>  }
>>
>>
>
> After you fixed that single problem I found, you can add:
>
> Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
>
> for the whole patch series.

Thank you. I will fix the 'cur.val' issue and add your Reviewed-by to
the next version of the series.

Regards.