| Message ID | 1348829458-13812-2-git-send-email-javier.martin@vista-silicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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); } 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; }