Re: [PATCH v3 03/22] media: imx219: Set horizontal blanking on mode change

[Dave Stevenson <dave.stevenson@raspberrypi.com>]

Hi Sakari

On Wed, 25 Mar 2026 at 10:58, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> The driver UAPI is mode-based, allowing the user to choose a mode from a
> small list based on the output size. The vertical blanking is set based on
> the mode, do the same for horizontal blanking so the frame rate obtained
> is constant.
>
> Additinally, it's best to use a known-good horizontal blanking value as

s/Additinally/Additionally

> choosing the value freely may affect image quality. While the minimum
> value may not be the best value for horizontal blanking, at least it is
> constant rather than a minimum value of a different configuration.

I've never known what the preferred behaviour is here. Ranges
typically change on mode change, and v4l2_ctrl_modify_range will reset
to the default if the current value is out of range, but otherwise
leave things alone.

Seeing as you would be the one defining the preferred behaviour, I'll
take this desire as gospel.

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

With the typo corrected:
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/media/i2c/imx219.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 89061dc1842d..62a23541b1dc 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -837,11 +837,9 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>         struct v4l2_mbus_framefmt *format;
>         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;
>
>         /*
>          * Adjust the requested format to match the closest mode. The Bayer
> @@ -882,7 +880,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>         if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>                 int exposure_max;
>                 int exposure_def;
> -               int hblank, llp_min;
> +               int llp_min;
>                 int pixel_rate;
>
>                 /* Update limits and set FPS to default */
> @@ -924,15 +922,8 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>                                                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.
> -                * Limits have to be recomputed as the controls define
> -                * the blanking only, so PPL values need to have the
> -                * mode width subtracted.
> -                */
> -               hblank = prev_line_len - mode->width;
> -               ret = __v4l2_ctrl_s_ctrl(imx219->hblank, hblank);
> +
> +               ret = __v4l2_ctrl_s_ctrl(imx219->hblank, llp_min - mode->width);
>                 if (ret)
>                         return ret;
>
> --
> 2.47.3
>