Messages in this thread | | | From | Rui Miguel Silva <> | Subject | Re: [PATCH v3 2/2] media: ov2680: Add Omnivision OV2680 sensor driver | Date | Tue, 27 Mar 2018 13:39:40 +0100 |
| |
Hi Sakari, On Sat 24 Mar 2018 at 10:57, Sakari Ailus wrote: > Hi Rui, > > I wanted to go through the code the last time and I'd have a few > review > comments below...
Thanks for the review.
> > 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.
Ok, no problem, will change that in v4.
> > ... > >> +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.
Yeah, I could definitely change the values of exposure and gain also, but that may came how the ctrl group work internally. But I will change and add them.
> >> + 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.
Yes, good catch.
> >> + >> + 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.
Makes sense, will change also in v4 that will go shortly.
--- Cheers, Rui
| |