Hi Guoniu Zhou,
Thank you for the patch.
On Fri, Dec 02, 2022 at 06:42:50PM +0800, G.N. Zhou (OSS) wrote:
> From: "Guoniu.zhou" <guoniu.zhou@xxxxxxx>
>
> In commit 3145efcdb4d0 ("media: ov5640: Rework timings programming"),
> it defines max_fps field in ov5640_mode_info structure to store maximum
> frame rate supported by each mode. But in ov5640_try_frame_interval(),it
s/,it/, it/
> assumes the maximum frame rate supported by all modes is 60. But actully,
s/actully/actually/
> only VGA support it. For others, the maximum frame rate supported is 30.
> So correct it by taking the maximum frame rate supported by each mode as
> the initialization value of the local variable maxfps.
>
> Signed-off-by: Guoniu.zhou <guoniu.zhou@xxxxxxx>
> ---
> drivers/media/i2c/ov5640.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 3f6d715efa82..a396f3eaf054 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -2722,13 +2722,17 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
> int minfps, maxfps, best_fps, fps;
> int i;
>
> + mode = ov5640_find_mode(sensor, width, height, false);
> + if (!mode)
> + return -EINVAL;
There are two callers of this function, ov5640_enum_frame_interval() and
ov5640_s_frame_interval(). The latter already has a mode pointer (taken
from sensor->current_mode). I would thus move the ov5640_find_mode()
call to ov5640_enum_frame_interval(), and pass the mode to
ov5640_try_frame_interval.
On a side note, the ov5640_find_mode() call in ov5640_s_frame_interval()
after ov5640_try_frame_interval() seems unneeded to me, it looks like it
could be dropped (in a separate patch).
> +
> minfps = ov5640_framerates[OV5640_15_FPS];
> - maxfps = ov5640_framerates[OV5640_60_FPS];
> + maxfps = ov5640_framerates[mode->max_fps];
>
> if (fi->numerator == 0) {
> fi->denominator = maxfps;
> fi->numerator = 1;
> - rate = OV5640_60_FPS;
> + rate = mode->max_fps;
> goto find_mode;
> }
>
--
Regards,
Laurent Pinchart
