Hi Hans
On Tue, Jun 27, 2023 at 03:18:08PM +0200, Hans de Goede wrote:
> ov2680_set_fmt() which == V4L2_SUBDEV_FORMAT_TRY was getting
> the try_fmt v4l2_mbus_framefmt struct from the passed in sd_state
> and then storing the contents of that into the return by reference
> format->format struct.
>
> While the right thing to do would be filling format->format based on
> the just looked up mode and then store the results of that in
> sd_state->pads[0].try_fmt .
>
> Before the previous change introducing ov2680_fill_format() this
> resulted in ov2680_set_fmt() which == V4L2_SUBDEV_FORMAT_TRY always
> returning the zero-ed out sd_state->pads[0].try_fmt in format->format
> breaking callers using this.
>
> After the introduction of ov2680_fill_format() which at least
> initializes sd_state->pads[0].try_fmt properly, format->format
> is now always being filled with the default 800x600 mode set by
> ov2680_init_cfg() independent of the actual requested mode.
>
> Move the filling of format->format with ov2680_fill_format() to
> before the if (which == V4L2_SUBDEV_FORMAT_TRY) and then store
> the filled in format->format in sd_state->pads[0].try_fmt to
> fix this.
>
> Note this removes the fmt local variable because IMHO having a local
> variable which points to a sub-struct of one of the function arguments
> just leads to confusion when reading the code.
>
> Fixes: 3ee47cad3e69 ("media: ov2680: Add Omnivision OV2680 sensor driver")
> Acked-by: Rui Miguel Silva <rmfrfs@xxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> drivers/media/i2c/ov2680.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> index c4a46c734d82..7fc4b39ebb37 100644
> --- a/drivers/media/i2c/ov2680.c
> +++ b/drivers/media/i2c/ov2680.c
> @@ -603,7 +603,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
> struct v4l2_subdev_format *format)
> {
> struct ov2680_dev *sensor = to_ov2680_dev(sd);
> - struct v4l2_mbus_framefmt *fmt = &format->format;
> struct v4l2_mbus_framefmt *try_fmt;
> const struct ov2680_mode_info *mode;
> int ret = 0;
> @@ -612,14 +611,18 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
> return -EINVAL;
>
> mode = v4l2_find_nearest_size(ov2680_mode_data,
> - ARRAY_SIZE(ov2680_mode_data), width,
> - height, fmt->width, fmt->height);
> + ARRAY_SIZE(ov2680_mode_data),
> + width, height,
> + format->format.width,
> + format->format.height);
> if (!mode)
> return -EINVAL;
Nit: only if you have to resend, could this be dropped? mode will be NULL
only if ov2680_mode_data[] has no entries.
>
> + ov2680_fill_format(sensor, &format->format, mode->width, mode->height);
> +
> if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> try_fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
> - format->format = *try_fmt;
> + *try_fmt = format->format;
> return 0;
> }
>
> @@ -630,8 +633,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
> goto unlock;
> }
>
> - ov2680_fill_format(sensor, fmt, mode->width, mode->height);
> -
> sensor->current_mode = mode;
> sensor->fmt = format->format;
> sensor->mode_pending_changes = true;
> --
> 2.41.0
>
