* [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
* [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 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 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
* 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
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®