Re: [PATCH] media: ov5640: report correct frame rate to user

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@nxp.com>
> 
> 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@nxp.com>
> ---
>  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