On 01/21/2011 04:09 AM, Hans Verkuil wrote: > Hi Sylwester! > > I have some review comments below... > > On Thursday, January 20, 2011 02:44:01 Sylwester Nawrocki wrote: >> Implement controls using the control framework. >> Add horizontal/vertical flip controls, minor cleanup. >> >> Signed-off-by: Sylwester Nawrocki<s.nawrocki@xxxxxxxxxxx> >> Signed-off-by: Kyungmin Park<kyungmin.park@xxxxxxxxxxx> >> --- >> drivers/media/video/sr030pc30.c | 311 +++++++++++++++++---------------------- >> 1 files changed, 132 insertions(+), 179 deletions(-) >> ... >> - >> -static int sr030pc30_s_ctrl(struct v4l2_subdev *sd, >> - struct v4l2_control *ctrl) >> +static int sr030pc30_s_ctrl(struct v4l2_ctrl *ctrl) >> { >> - int i, ret = 0; >> - >> - for (i = 0; i< ARRAY_SIZE(sr030pc30_ctrl); i++) >> - if (ctrl->id == sr030pc30_ctrl[i].id) >> - break; >> - >> - if (i == ARRAY_SIZE(sr030pc30_ctrl)) >> - return -EINVAL; >> - >> - if (ctrl->value< sr030pc30_ctrl[i].minimum || >> - ctrl->value> sr030pc30_ctrl[i].maximum) >> - return -ERANGE; >> + struct v4l2_subdev *sd = to_sd(ctrl); >> + struct sr030pc30_info *info = to_sr030pc30(sd); >> + int ret = 0; >> >> - v4l2_dbg(1, debug, sd, "%s: ctrl_id: %d, value: %d\n", >> - __func__, ctrl->id, ctrl->value); >> + v4l2_dbg(1, debug, sd, "%s: ctrl id: %d, value: %d\n", >> + __func__, ctrl->id, ctrl->val); >> >> switch (ctrl->id) { >> case V4L2_CID_AUTO_WHITE_BALANCE: >> - sr030pc30_enable_autowhitebalance(sd, ctrl->value); >> - break; >> - case V4L2_CID_BLUE_BALANCE: >> - ret = sr030pc30_set_bluebalance(sd, ctrl->value); >> - break; >> - case V4L2_CID_RED_BALANCE: >> - ret = sr030pc30_set_redbalance(sd, ctrl->value); >> - break; >> - case V4L2_CID_EXPOSURE_AUTO: >> - sr030pc30_enable_autoexposure(sd, >> - ctrl->value == V4L2_EXPOSURE_AUTO); >> - break; >> - case V4L2_CID_EXPOSURE: >> - ret = sr030pc30_set_exposure(sd, ctrl->value); >> - break; >> - default: >> - return -EINVAL; >> - } >> + if (!ctrl->has_new) >> + ctrl->val = 0; >> >> - return ret; >> -} >> - >> -static int sr030pc30_g_ctrl(struct v4l2_subdev *sd, >> - struct v4l2_control *ctrl) >> -{ >> - struct sr030pc30_info *info = to_sr030pc30(sd); >> + ret = sr030pc30_enable_autowhitebalance(sd, ctrl->val); >> >> - v4l2_dbg(1, debug, sd, "%s: id: %d\n", __func__, ctrl->id); >> + if (!ret&& !ctrl->val) { >> + ret = cam_i2c_write(sd, MWB_BGAIN_REG, info->blue->val); >> + if (!ret) >> + ret = cam_i2c_write(sd, MWB_RGAIN_REG, >> + info->red->val); >> + } >> + return ret; >> >> - switch (ctrl->id) { >> - case V4L2_CID_AUTO_WHITE_BALANCE: >> - ctrl->value = info->auto_wb; >> - break; >> - case V4L2_CID_BLUE_BALANCE: >> - ctrl->value = info->blue_balance; >> - break; >> - case V4L2_CID_RED_BALANCE: >> - ctrl->value = info->red_balance; >> - break; >> case V4L2_CID_EXPOSURE_AUTO: >> - ctrl->value = info->auto_exp; >> - break; >> - case V4L2_CID_EXPOSURE: >> - ctrl->value = info->exposure; >> - break; >> + if (!ctrl->has_new) >> + ctrl->val = V4L2_EXPOSURE_MANUAL; >> + >> + if (ctrl->val == V4L2_EXPOSURE_MANUAL) >> + return sr030pc30_set_exposure(sd, info->exposure->val); >> + else >> + return sr030pc30_enable_autoexposure(sd, 1); > > Setting up auto<foo> and<foo> controls like this is a bit tricky. I've > converted several drivers recently and had to do quite a few of these. Based > on that experience I will be posting a RFC patch this weekend adding special > support for this to the control framework that simplifies life a bit. > > So I'd appreciate it if you could test those changes with this driver and if > all is OK, then I'll make a pull request for that and you can base your > changes on top of that. > I have converted sr030pc30 already just need to do testing when I get my hands on the hardware. With your foo/auto-foo for V4L2_CID_EXPOSURE_AUTO I did: case V4L2_CID_EXPOSURE_AUTO: if (ctrl->val == V4L2_EXPOSURE_MANUAL) ret = sr030pc30_set_exposure(sd, info->exposure->val); /* Set autoexposure and auto anti-flicker. */ if (!ret) return cam_i2c_write(sd, AE_CTL1_REG, ctrl->val == V4L2_EXPOSURE_MANUAL ? 0xDC : 0x0C); return ret; Is the change with foo/auto-foo controls support in v4l2 core only about that the "is_new" flag don't have to be checked in s_ctrl for each control and ctrl->val reassigned there? Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html