Hi Paul, On Wed, Feb 26, 2025 at 05:49:07PM +0900, Paul Elder wrote: > Hi Sakari, > > Thanks for the review. > > On Sun, Feb 16, 2025 at 10:01:27AM +0000, Sakari Ailus wrote: > > Hi Paul, > > > > On Fri, Feb 14, 2025 at 10:37:09PM +0900, Paul Elder wrote: > > > When the vblank v4l2 control is set, it does not get written to the > > > hardware immediately. It only gets updated when exposure is set. Change > > > the behavior such that the vblank is written immediately when the > > > control is set. > > > > > > Signed-off-by: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx> > > > --- > > > drivers/media/i2c/imx335.c | 19 +++++++++++++------ > > > 1 file changed, 13 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c > > > index fcfd1d851bd4..e73a23bbbc89 100644 > > > --- a/drivers/media/i2c/imx335.c > > > +++ b/drivers/media/i2c/imx335.c > > > @@ -559,12 +559,12 @@ static int imx335_set_ctrl(struct v4l2_ctrl *ctrl) > > > imx335->vblank, > > > imx335->vblank + imx335->cur_mode->height); > > > > > > - return __v4l2_ctrl_modify_range(imx335->exp_ctrl, > > > - IMX335_EXPOSURE_MIN, > > > - imx335->vblank + > > > - imx335->cur_mode->height - > > > - IMX335_EXPOSURE_OFFSET, > > > - 1, IMX335_EXPOSURE_DEFAULT); > > > + __v4l2_ctrl_modify_range(imx335->exp_ctrl, > > > > Indentation. > > > > You're also missing an error check here. > > I reasoned that it's fine to not have the error check. > > afaiu, the only change this has to error is if try/s_ctrl on > V4L2_CID_EXPOSURE fails when the change to the range of valid exposure > values requires a new exposure value to be set. Setting the exposure > control comes back to this function, and goes through the switch-case > and imx335_update_exp_gain() below, which doesn't fail. It will fail if cci_write() it calls does. > > Also the imx219 has the exact same pattern in imx219_set_ctrl. Feel free to fix it. :-) -- Kind regards, Sakari Ailus
