* [PATCH v1 0/2] media: i2c: imx219: Improve error handling and readability @ 2025-10-31 11:48 Tarang Raval 2025-10-31 11:48 ` [PATCH v1 1/2] media: i2c: imx219: Propagate errors from control range updates Tarang Raval 2025-10-31 11:48 ` [PATCH v1 2/2] media: i2c: imx219: Replace exposure magic value with named constant Tarang Raval 0 siblings, 2 replies; 8+ messages in thread From: Tarang Raval @ 2025-10-31 11:48 UTC (permalink / raw) To: sakari.ailus Cc: Tarang Raval, Dave Stevenson, Mauro Carvalho Chehab, linux-media, linux-kernel This series improves the IMX219 driver by adding proper error propagation for control range updates and replacing a hardcoded exposure value with a named constant as per the datasheet. Tarang Raval (2): media: i2c: imx219: Propagate errors from control range updates media: i2c: imx219: Replace exposure magic value with named constant drivers/media/i2c/imx219.c | 74 ++++++++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 27 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v1 1/2] media: i2c: imx219: Propagate errors from control range updates 2025-10-31 11:48 [PATCH v1 0/2] media: i2c: imx219: Improve error handling and readability Tarang Raval @ 2025-10-31 11:48 ` Tarang Raval 2025-11-03 18:17 ` Dave Stevenson 2025-11-04 15:21 ` Isaac Scott 2025-10-31 11:48 ` [PATCH v1 2/2] media: i2c: imx219: Replace exposure magic value with named constant Tarang Raval 1 sibling, 2 replies; 8+ messages in thread From: Tarang Raval @ 2025-10-31 11:48 UTC (permalink / raw) To: sakari.ailus Cc: Tarang Raval, Dave Stevenson, Mauro Carvalho Chehab, linux-media, linux-kernel Propagate return values from __v4l2_ctrl_modify_range() and __v4l2_ctrl_s_ctrl() in imx219_set_ctrl() and imx219_set_pad_format(). This ensures proper error handling instead of ignoring possible failures. Also return the result of imx219_set_pad_format() from imx219_init_state(). Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io> --- drivers/media/i2c/imx219.c | 61 +++++++++++++++++++++++++------------- 1 file changed, 40 insertions(+), 21 deletions(-) diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c index 48efdcd2a8f9..40693635c0c3 100644 --- a/drivers/media/i2c/imx219.c +++ b/drivers/media/i2c/imx219.c @@ -453,10 +453,14 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) exposure_max = format->height + ctrl->val - 4; exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? exposure_max : IMX219_EXPOSURE_DEFAULT; - __v4l2_ctrl_modify_range(imx219->exposure, - imx219->exposure->minimum, - exposure_max, imx219->exposure->step, - exposure_def); + ret = __v4l2_ctrl_modify_range(imx219->exposure, + imx219->exposure->minimum, + exposure_max, + imx219->exposure->step, + exposure_def); + if (ret) + return ret; + } /* @@ -848,6 +852,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, struct v4l2_rect *crop; u8 bin_h, bin_v, binning; u32 prev_line_len; + int ret; format = v4l2_subdev_state_get_format(state, 0); prev_line_len = format->width + imx219->hblank->val; @@ -883,19 +888,28 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, int pixel_rate; /* Update limits and set FPS to default */ - __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN, - IMX219_FLL_MAX - mode->height, 1, + ret = __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN, + IMX219_FLL_MAX - mode->height, 1, + mode->fll_def - mode->height); + if (ret) + return ret; + + ret = __v4l2_ctrl_s_ctrl(imx219->vblank, mode->fll_def - mode->height); - __v4l2_ctrl_s_ctrl(imx219->vblank, - mode->fll_def - mode->height); + if (ret) + return ret; + /* Update max exposure while meeting expected vblanking */ exposure_max = mode->fll_def - 4; exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? exposure_max : IMX219_EXPOSURE_DEFAULT; - __v4l2_ctrl_modify_range(imx219->exposure, - imx219->exposure->minimum, - exposure_max, imx219->exposure->step, - exposure_def); + ret = __v4l2_ctrl_modify_range(imx219->exposure, + imx219->exposure->minimum, + exposure_max, + imx219->exposure->step, + exposure_def); + if (ret) + return ret; /* * With analog binning the default minimum line length of 3448 @@ -906,9 +920,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, imx219_get_binning(state, &bin_h, &bin_v); llp_min = (bin_h & bin_v) == IMX219_BINNING_X2_ANALOG ? IMX219_BINNED_LLP_MIN : IMX219_LLP_MIN; - __v4l2_ctrl_modify_range(imx219->hblank, llp_min - mode->width, - IMX219_LLP_MAX - mode->width, 1, - llp_min - mode->width); + ret = __v4l2_ctrl_modify_range(imx219->hblank, + llp_min - mode->width, + IMX219_LLP_MAX - mode->width, 1, + llp_min - mode->width); + if (ret) + return ret; /* * Retain PPL setting from previous mode so that the * line time does not change on a mode change. @@ -917,13 +934,17 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, * mode width subtracted. */ hblank = prev_line_len - mode->width; - __v4l2_ctrl_s_ctrl(imx219->hblank, hblank); + ret = __v4l2_ctrl_s_ctrl(imx219->hblank, hblank); + if (ret) + return ret; /* Scale the pixel rate based on the mode specific factor */ pixel_rate = imx219_get_pixel_rate(imx219) * imx219_get_rate_factor(state); - __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate, - pixel_rate, 1, pixel_rate); + ret = __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate, + pixel_rate, 1, pixel_rate); + if (ret) + return ret; } return 0; @@ -972,9 +993,7 @@ static int imx219_init_state(struct v4l2_subdev *sd, }, }; - imx219_set_pad_format(sd, state, &fmt); - - return 0; + return imx219_set_pad_format(sd, state, &fmt); } static const struct v4l2_subdev_video_ops imx219_video_ops = { -- 2.34.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/2] media: i2c: imx219: Propagate errors from control range updates 2025-10-31 11:48 ` [PATCH v1 1/2] media: i2c: imx219: Propagate errors from control range updates Tarang Raval @ 2025-11-03 18:17 ` Dave Stevenson 2025-11-04 7:04 ` Tarang Raval 2025-11-04 15:21 ` Isaac Scott 1 sibling, 1 reply; 8+ messages in thread From: Dave Stevenson @ 2025-11-03 18:17 UTC (permalink / raw) To: Tarang Raval Cc: sakari.ailus, Mauro Carvalho Chehab, linux-media, linux-kernel Hi Tarang On Fri, 31 Oct 2025 at 11:49, Tarang Raval <tarang.raval@siliconsignals.io> wrote: > > Propagate return values from __v4l2_ctrl_modify_range() and > __v4l2_ctrl_s_ctrl() in imx219_set_ctrl() and imx219_set_pad_format(). > This ensures proper error handling instead of ignoring possible > failures. Also return the result of imx219_set_pad_format() from > imx219_init_state(). It should be impossible for a number of these to fail unless someone has messed up in updating the driver, but it does little harm in returning the error code back out. The slight hesitation would be that in imx219_set_pad_format you could have updated the ranges/values of one or more controls, and then fail leaving a partially implemented mode change. It has returned an error, but an application would be reasonable in thinking that the previous working state has been retained when it hasn't. As long as it would only trigger due to a driver bug rather than user interaction, I would *not* propose that all values have to be saved and have to be restored on failure. It just gets too ugly. > Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/media/i2c/imx219.c | 61 +++++++++++++++++++++++++------------- > 1 file changed, 40 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index 48efdcd2a8f9..40693635c0c3 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -453,10 +453,14 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > exposure_max = format->height + ctrl->val - 4; > exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? > exposure_max : IMX219_EXPOSURE_DEFAULT; > - __v4l2_ctrl_modify_range(imx219->exposure, > - imx219->exposure->minimum, > - exposure_max, imx219->exposure->step, > - exposure_def); > + ret = __v4l2_ctrl_modify_range(imx219->exposure, > + imx219->exposure->minimum, > + exposure_max, > + imx219->exposure->step, > + exposure_def); > + if (ret) > + return ret; > + > } > > /* > @@ -848,6 +852,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > struct v4l2_rect *crop; > u8 bin_h, bin_v, binning; > u32 prev_line_len; > + int ret; > > format = v4l2_subdev_state_get_format(state, 0); > prev_line_len = format->width + imx219->hblank->val; > @@ -883,19 +888,28 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > int pixel_rate; > > /* Update limits and set FPS to default */ > - __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN, > - IMX219_FLL_MAX - mode->height, 1, > + ret = __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN, > + IMX219_FLL_MAX - mode->height, 1, > + mode->fll_def - mode->height); > + if (ret) > + return ret; > + > + ret = __v4l2_ctrl_s_ctrl(imx219->vblank, > mode->fll_def - mode->height); > - __v4l2_ctrl_s_ctrl(imx219->vblank, > - mode->fll_def - mode->height); > + if (ret) > + return ret; > + > /* Update max exposure while meeting expected vblanking */ > exposure_max = mode->fll_def - 4; > exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? > exposure_max : IMX219_EXPOSURE_DEFAULT; > - __v4l2_ctrl_modify_range(imx219->exposure, > - imx219->exposure->minimum, > - exposure_max, imx219->exposure->step, > - exposure_def); > + ret = __v4l2_ctrl_modify_range(imx219->exposure, > + imx219->exposure->minimum, > + exposure_max, > + imx219->exposure->step, > + exposure_def); > + if (ret) > + return ret; > > /* > * With analog binning the default minimum line length of 3448 > @@ -906,9 +920,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > imx219_get_binning(state, &bin_h, &bin_v); > llp_min = (bin_h & bin_v) == IMX219_BINNING_X2_ANALOG ? > IMX219_BINNED_LLP_MIN : IMX219_LLP_MIN; > - __v4l2_ctrl_modify_range(imx219->hblank, llp_min - mode->width, > - IMX219_LLP_MAX - mode->width, 1, > - llp_min - mode->width); > + ret = __v4l2_ctrl_modify_range(imx219->hblank, > + llp_min - mode->width, > + IMX219_LLP_MAX - mode->width, 1, > + llp_min - mode->width); > + if (ret) > + return ret; > /* > * Retain PPL setting from previous mode so that the > * line time does not change on a mode change. > @@ -917,13 +934,17 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > * mode width subtracted. > */ > hblank = prev_line_len - mode->width; > - __v4l2_ctrl_s_ctrl(imx219->hblank, hblank); > + ret = __v4l2_ctrl_s_ctrl(imx219->hblank, hblank); > + if (ret) > + return ret; > > /* Scale the pixel rate based on the mode specific factor */ > pixel_rate = imx219_get_pixel_rate(imx219) * > imx219_get_rate_factor(state); > - __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate, > - pixel_rate, 1, pixel_rate); > + ret = __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate, > + pixel_rate, 1, pixel_rate); > + if (ret) > + return ret; > } > > return 0; > @@ -972,9 +993,7 @@ static int imx219_init_state(struct v4l2_subdev *sd, > }, > }; > > - imx219_set_pad_format(sd, state, &fmt); > - > - return 0; > + return imx219_set_pad_format(sd, state, &fmt); > } > > static const struct v4l2_subdev_video_ops imx219_video_ops = { > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/2] media: i2c: imx219: Propagate errors from control range updates 2025-11-03 18:17 ` Dave Stevenson @ 2025-11-04 7:04 ` Tarang Raval 0 siblings, 0 replies; 8+ messages in thread From: Tarang Raval @ 2025-11-04 7:04 UTC (permalink / raw) To: Dave Stevenson Cc: sakari.ailus, Mauro Carvalho Chehab, linux-media, linux-kernel Hi Dave, > On Fri, 31 Oct 2025 at 11:49, Tarang Raval > <tarang.raval@siliconsignals.io> wrote: > > > > Propagate return values from __v4l2_ctrl_modify_range() and > > __v4l2_ctrl_s_ctrl() in imx219_set_ctrl() and imx219_set_pad_format(). > > This ensures proper error handling instead of ignoring possible > > failures. Also return the result of imx219_set_pad_format() from > > imx219_init_state(). > > It should be impossible for a number of these to fail unless someone > has messed up in updating the driver, but it does little harm in > returning the error code back out. > > The slight hesitation would be that in imx219_set_pad_format you could > have updated the ranges/values of one or more controls, and then fail > leaving a partially implemented mode change. It has returned an error, > but an application would be reasonable in thinking that the previous > working state has been retained when it hasn't. > As long as it would only trigger due to a driver bug rather than user > interaction, I would *not* propose that all values have to be saved > and have to be restored on failure. It just gets too ugly. Thank you for the detailed feedback and insights. I completely agree with your assessment regarding the potential for partial updates in the imx219_set_pad_format() function. As you mentioned, such failures should only occur due to a driver bug, so this change mainly serves as protection against that scenario. Returning the error codes ensures better visibility of unexpected issues and makes debugging easier if a regression ever occurs. > > Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io> > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> Thanks! Best Regards, Tarang ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/2] media: i2c: imx219: Propagate errors from control range updates 2025-10-31 11:48 ` [PATCH v1 1/2] media: i2c: imx219: Propagate errors from control range updates Tarang Raval 2025-11-03 18:17 ` Dave Stevenson @ 2025-11-04 15:21 ` Isaac Scott 1 sibling, 0 replies; 8+ messages in thread From: Isaac Scott @ 2025-11-04 15:21 UTC (permalink / raw) To: Tarang Raval, sakari.ailus Cc: Tarang Raval, Dave Stevenson, Mauro Carvalho Chehab, linux-media, linux-kernel Hi Tarang, Thank you for the patch! Quoting Tarang Raval (2025-10-31 11:48:34) > Propagate return values from __v4l2_ctrl_modify_range() and > __v4l2_ctrl_s_ctrl() in imx219_set_ctrl() and imx219_set_pad_format(). > This ensures proper error handling instead of ignoring possible > failures. Also return the result of imx219_set_pad_format() from > imx219_init_state(). > > Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io> Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com> > --- > drivers/media/i2c/imx219.c | 61 +++++++++++++++++++++++++------------- > 1 file changed, 40 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index 48efdcd2a8f9..40693635c0c3 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -453,10 +453,14 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > exposure_max = format->height + ctrl->val - 4; > exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? > exposure_max : IMX219_EXPOSURE_DEFAULT; > - __v4l2_ctrl_modify_range(imx219->exposure, > - imx219->exposure->minimum, > - exposure_max, imx219->exposure->step, > - exposure_def); > + ret = __v4l2_ctrl_modify_range(imx219->exposure, > + imx219->exposure->minimum, > + exposure_max, > + imx219->exposure->step, > + exposure_def); > + if (ret) > + return ret; > + > } > > /* > @@ -848,6 +852,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > struct v4l2_rect *crop; > u8 bin_h, bin_v, binning; > u32 prev_line_len; > + int ret; > > format = v4l2_subdev_state_get_format(state, 0); > prev_line_len = format->width + imx219->hblank->val; > @@ -883,19 +888,28 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > int pixel_rate; > > /* Update limits and set FPS to default */ > - __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN, > - IMX219_FLL_MAX - mode->height, 1, > + ret = __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN, > + IMX219_FLL_MAX - mode->height, 1, > + mode->fll_def - mode->height); > + if (ret) > + return ret; > + > + ret = __v4l2_ctrl_s_ctrl(imx219->vblank, > mode->fll_def - mode->height); > - __v4l2_ctrl_s_ctrl(imx219->vblank, > - mode->fll_def - mode->height); > + if (ret) > + return ret; > + > /* Update max exposure while meeting expected vblanking */ > exposure_max = mode->fll_def - 4; > exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? > exposure_max : IMX219_EXPOSURE_DEFAULT; > - __v4l2_ctrl_modify_range(imx219->exposure, > - imx219->exposure->minimum, > - exposure_max, imx219->exposure->step, > - exposure_def); > + ret = __v4l2_ctrl_modify_range(imx219->exposure, > + imx219->exposure->minimum, > + exposure_max, > + imx219->exposure->step, > + exposure_def); > + if (ret) > + return ret; > > /* > * With analog binning the default minimum line length of 3448 > @@ -906,9 +920,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > imx219_get_binning(state, &bin_h, &bin_v); > llp_min = (bin_h & bin_v) == IMX219_BINNING_X2_ANALOG ? > IMX219_BINNED_LLP_MIN : IMX219_LLP_MIN; > - __v4l2_ctrl_modify_range(imx219->hblank, llp_min - mode->width, > - IMX219_LLP_MAX - mode->width, 1, > - llp_min - mode->width); > + ret = __v4l2_ctrl_modify_range(imx219->hblank, > + llp_min - mode->width, > + IMX219_LLP_MAX - mode->width, 1, > + llp_min - mode->width); > + if (ret) > + return ret; > /* > * Retain PPL setting from previous mode so that the > * line time does not change on a mode change. > @@ -917,13 +934,17 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > * mode width subtracted. > */ > hblank = prev_line_len - mode->width; > - __v4l2_ctrl_s_ctrl(imx219->hblank, hblank); > + ret = __v4l2_ctrl_s_ctrl(imx219->hblank, hblank); > + if (ret) > + return ret; > > /* Scale the pixel rate based on the mode specific factor */ > pixel_rate = imx219_get_pixel_rate(imx219) * > imx219_get_rate_factor(state); > - __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate, > - pixel_rate, 1, pixel_rate); > + ret = __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate, > + pixel_rate, 1, pixel_rate); > + if (ret) > + return ret; > } > > return 0; > @@ -972,9 +993,7 @@ static int imx219_init_state(struct v4l2_subdev *sd, > }, > }; > > - imx219_set_pad_format(sd, state, &fmt); > - > - return 0; > + return imx219_set_pad_format(sd, state, &fmt); > } > > static const struct v4l2_subdev_video_ops imx219_video_ops = { > -- > 2.34.1 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v1 2/2] media: i2c: imx219: Replace exposure magic value with named constant 2025-10-31 11:48 [PATCH v1 0/2] media: i2c: imx219: Improve error handling and readability Tarang Raval 2025-10-31 11:48 ` [PATCH v1 1/2] media: i2c: imx219: Propagate errors from control range updates Tarang Raval @ 2025-10-31 11:48 ` Tarang Raval 2025-10-31 15:08 ` Kieran Bingham 2025-11-03 18:19 ` Dave Stevenson 1 sibling, 2 replies; 8+ messages in thread From: Tarang Raval @ 2025-10-31 11:48 UTC (permalink / raw) To: sakari.ailus Cc: Tarang Raval, Dave Stevenson, Mauro Carvalho Chehab, linux-media, linux-kernel Introduce IMX219_EXPOSURE_OFFSET (4) and use it instead of the literal '4' when computing the maximum coarse exposure. The IMX219 datasheet specifies the maximum storage time as frame_length_lines - 4. (Ref: Datasheet section 5-7-1) Also fix one indentation issue for consistency. Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io> --- drivers/media/i2c/imx219.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c index 40693635c0c3..e87d5a18fe87 100644 --- a/drivers/media/i2c/imx219.c +++ b/drivers/media/i2c/imx219.c @@ -68,6 +68,7 @@ #define IMX219_EXPOSURE_STEP 1 #define IMX219_EXPOSURE_DEFAULT 0x640 #define IMX219_EXPOSURE_MAX 65535 +#define IMX219_EXPOSURE_OFFSET 4 /* V_TIMING internal */ #define IMX219_REG_FRM_LENGTH_A CCI_REG16(0x0160) @@ -450,9 +451,9 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) int exposure_max, exposure_def; /* Update max exposure while meeting expected vblanking */ - exposure_max = format->height + ctrl->val - 4; + exposure_max = format->height + ctrl->val - IMX219_EXPOSURE_OFFSET; exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? - exposure_max : IMX219_EXPOSURE_DEFAULT; + exposure_max : IMX219_EXPOSURE_DEFAULT; ret = __v4l2_ctrl_modify_range(imx219->exposure, imx219->exposure->minimum, exposure_max, @@ -579,9 +580,9 @@ static int imx219_init_controls(struct imx219 *imx219) IMX219_LLP_MIN - mode->width, IMX219_LLP_MAX - mode->width, 1, IMX219_LLP_MIN - mode->width); - exposure_max = mode->fll_def - 4; + exposure_max = mode->fll_def - IMX219_EXPOSURE_OFFSET; exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? - exposure_max : IMX219_EXPOSURE_DEFAULT; + exposure_max : IMX219_EXPOSURE_DEFAULT; imx219->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, V4L2_CID_EXPOSURE, IMX219_EXPOSURE_MIN, exposure_max, @@ -900,9 +901,9 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, return ret; /* Update max exposure while meeting expected vblanking */ - exposure_max = mode->fll_def - 4; + exposure_max = mode->fll_def - IMX219_EXPOSURE_OFFSET; exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? - exposure_max : IMX219_EXPOSURE_DEFAULT; + exposure_max : IMX219_EXPOSURE_DEFAULT; ret = __v4l2_ctrl_modify_range(imx219->exposure, imx219->exposure->minimum, exposure_max, -- 2.34.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 2/2] media: i2c: imx219: Replace exposure magic value with named constant 2025-10-31 11:48 ` [PATCH v1 2/2] media: i2c: imx219: Replace exposure magic value with named constant Tarang Raval @ 2025-10-31 15:08 ` Kieran Bingham 2025-11-03 18:19 ` Dave Stevenson 1 sibling, 0 replies; 8+ messages in thread From: Kieran Bingham @ 2025-10-31 15:08 UTC (permalink / raw) To: Tarang Raval, sakari.ailus Cc: Tarang Raval, Dave Stevenson, Mauro Carvalho Chehab, linux-media, linux-kernel Quoting Tarang Raval (2025-10-31 11:48:35) > Introduce IMX219_EXPOSURE_OFFSET (4) and use it instead of the literal > '4' when computing the maximum coarse exposure. The IMX219 datasheet > specifies the maximum storage time as frame_length_lines - 4. > (Ref: Datasheet section 5-7-1) > > Also fix one indentation issue for consistency. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io> > --- > drivers/media/i2c/imx219.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index 40693635c0c3..e87d5a18fe87 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -68,6 +68,7 @@ > #define IMX219_EXPOSURE_STEP 1 > #define IMX219_EXPOSURE_DEFAULT 0x640 > #define IMX219_EXPOSURE_MAX 65535 > +#define IMX219_EXPOSURE_OFFSET 4 > > /* V_TIMING internal */ > #define IMX219_REG_FRM_LENGTH_A CCI_REG16(0x0160) > @@ -450,9 +451,9 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > int exposure_max, exposure_def; > > /* Update max exposure while meeting expected vblanking */ > - exposure_max = format->height + ctrl->val - 4; > + exposure_max = format->height + ctrl->val - IMX219_EXPOSURE_OFFSET; > exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? > - exposure_max : IMX219_EXPOSURE_DEFAULT; > + exposure_max : IMX219_EXPOSURE_DEFAULT; > ret = __v4l2_ctrl_modify_range(imx219->exposure, > imx219->exposure->minimum, > exposure_max, > @@ -579,9 +580,9 @@ static int imx219_init_controls(struct imx219 *imx219) > IMX219_LLP_MIN - mode->width, > IMX219_LLP_MAX - mode->width, 1, > IMX219_LLP_MIN - mode->width); > - exposure_max = mode->fll_def - 4; > + exposure_max = mode->fll_def - IMX219_EXPOSURE_OFFSET; > exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? > - exposure_max : IMX219_EXPOSURE_DEFAULT; > + exposure_max : IMX219_EXPOSURE_DEFAULT; > imx219->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, > V4L2_CID_EXPOSURE, > IMX219_EXPOSURE_MIN, exposure_max, > @@ -900,9 +901,9 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > return ret; > > /* Update max exposure while meeting expected vblanking */ > - exposure_max = mode->fll_def - 4; > + exposure_max = mode->fll_def - IMX219_EXPOSURE_OFFSET; > exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? > - exposure_max : IMX219_EXPOSURE_DEFAULT; > + exposure_max : IMX219_EXPOSURE_DEFAULT; > ret = __v4l2_ctrl_modify_range(imx219->exposure, > imx219->exposure->minimum, > exposure_max, > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 2/2] media: i2c: imx219: Replace exposure magic value with named constant 2025-10-31 11:48 ` [PATCH v1 2/2] media: i2c: imx219: Replace exposure magic value with named constant Tarang Raval 2025-10-31 15:08 ` Kieran Bingham @ 2025-11-03 18:19 ` Dave Stevenson 1 sibling, 0 replies; 8+ messages in thread From: Dave Stevenson @ 2025-11-03 18:19 UTC (permalink / raw) To: Tarang Raval Cc: sakari.ailus, Mauro Carvalho Chehab, linux-media, linux-kernel Hi Tarang On Fri, 31 Oct 2025 at 11:49, Tarang Raval <tarang.raval@siliconsignals.io> wrote: > > Introduce IMX219_EXPOSURE_OFFSET (4) and use it instead of the literal > '4' when computing the maximum coarse exposure. The IMX219 datasheet > specifies the maximum storage time as frame_length_lines - 4. > (Ref: Datasheet section 5-7-1) > > Also fix one indentation issue for consistency. > > Signed-off-by: Tarang Raval <tarang.raval@siliconsignals.io> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/media/i2c/imx219.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index 40693635c0c3..e87d5a18fe87 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -68,6 +68,7 @@ > #define IMX219_EXPOSURE_STEP 1 > #define IMX219_EXPOSURE_DEFAULT 0x640 > #define IMX219_EXPOSURE_MAX 65535 > +#define IMX219_EXPOSURE_OFFSET 4 > > /* V_TIMING internal */ > #define IMX219_REG_FRM_LENGTH_A CCI_REG16(0x0160) > @@ -450,9 +451,9 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > int exposure_max, exposure_def; > > /* Update max exposure while meeting expected vblanking */ > - exposure_max = format->height + ctrl->val - 4; > + exposure_max = format->height + ctrl->val - IMX219_EXPOSURE_OFFSET; > exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? > - exposure_max : IMX219_EXPOSURE_DEFAULT; > + exposure_max : IMX219_EXPOSURE_DEFAULT; > ret = __v4l2_ctrl_modify_range(imx219->exposure, > imx219->exposure->minimum, > exposure_max, > @@ -579,9 +580,9 @@ static int imx219_init_controls(struct imx219 *imx219) > IMX219_LLP_MIN - mode->width, > IMX219_LLP_MAX - mode->width, 1, > IMX219_LLP_MIN - mode->width); > - exposure_max = mode->fll_def - 4; > + exposure_max = mode->fll_def - IMX219_EXPOSURE_OFFSET; > exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? > - exposure_max : IMX219_EXPOSURE_DEFAULT; > + exposure_max : IMX219_EXPOSURE_DEFAULT; > imx219->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops, > V4L2_CID_EXPOSURE, > IMX219_EXPOSURE_MIN, exposure_max, > @@ -900,9 +901,9 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > return ret; > > /* Update max exposure while meeting expected vblanking */ > - exposure_max = mode->fll_def - 4; > + exposure_max = mode->fll_def - IMX219_EXPOSURE_OFFSET; > exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? > - exposure_max : IMX219_EXPOSURE_DEFAULT; > + exposure_max : IMX219_EXPOSURE_DEFAULT; > ret = __v4l2_ctrl_modify_range(imx219->exposure, > imx219->exposure->minimum, > exposure_max, > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-11-04 15:21 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-10-31 11:48 [PATCH v1 0/2] media: i2c: imx219: Improve error handling and readability Tarang Raval 2025-10-31 11:48 ` [PATCH v1 1/2] media: i2c: imx219: Propagate errors from control range updates Tarang Raval 2025-11-03 18:17 ` Dave Stevenson 2025-11-04 7:04 ` Tarang Raval 2025-11-04 15:21 ` Isaac Scott 2025-10-31 11:48 ` [PATCH v1 2/2] media: i2c: imx219: Replace exposure magic value with named constant Tarang Raval 2025-10-31 15:08 ` Kieran Bingham 2025-11-03 18:19 ` Dave Stevenson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®