Hi Bingbu, Sergey,
On Tue, Nov 17, 2020 at 07:29:08PM +0800, Bingbu Cao wrote:
> Sergey, thanks for review.
>
> On 11/17/20 4:07 PM, Sergey Senozhatsky wrote:
> > On (20/10/29 10:59), Bingbu Cao wrote:
> > [..]
> >
> > Looks good to me,
> > Reviewed-by: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
> >
> > Several comments below.
> >
> >> +static int ov9734_set_ctrl(struct v4l2_ctrl *ctrl)
> >> +{
> >> + struct ov9734 *ov9734 = container_of(ctrl->handler,
> >> + struct ov9734, ctrl_handler);
> >> + struct i2c_client *client = v4l2_get_subdevdata(&ov9734->sd);
> >> + s64 exposure_max;
> >> + int ret = 0;
> >> +
> >> + /* Propagate change of current control to all related controls */
> >> + if (ctrl->id == V4L2_CID_VBLANK) {
> >> + /* Update max exposure while meeting expected vblanking */
> >> + exposure_max = ov9734->cur_mode->height + ctrl->val -
> >> + OV9734_EXPOSURE_MAX_MARGIN;
> >> + __v4l2_ctrl_modify_range(ov9734->exposure,
> >> + ov9734->exposure->minimum,
> >> + exposure_max, ov9734->exposure->step,
> >> + exposure_max);
> >> + }
> >
> > Should this be done after pm_runtime_get_if_in_use()?
>
> My inaccurate understanding - as v4l2 see that this control was set by sub-device without error,
> so it will record the value from the user as the new value, so update the exposure range to
> allow user to set the exposure as well even it did not take affect.
>
> Sakari, do you have any comments about this?
Just changing the range does not require powering on the device. So this is
as intended AFAIU.
--
Regards,
Sakari Ailus
