On Wed, Feb 26, 2025 at 08:56:06AM +0000, Sakari Ailus wrote: > 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. Hm good point. I suppose we don't actually want to continue on if it does fail... ok I'll fix it. > > > > > Also the imx219 has the exact same pattern in imx219_set_ctrl. > > Feel free to fix it. :-) :) Paul
