Re: [PATCH 15/21] media: ov5640: Limit format to FPS in DVP mode only

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

Hi Jacopo,

Thank you for the patch.

On Mon, Jan 31, 2022 at 03:44:43PM +0100, Jacopo Mondi wrote:
> In MIPI mode the frame rate control is performed by adjusting the
> frame blankings and the s_frame_interval function is not used anymore.
> 
> Only check for the per-mode supported frame rate in DVP mode and do not
> restrict MIPI mode.

This certainly aligns better with how the sensor driver is supposed to
operate. I however wonder why you don't do so in DVP mode too. Is it for
backward-compatibility ? If so a comment would be useful.

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/ov5640.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index ae22300b9655..ec46e16223af 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -1845,8 +1845,13 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
>  	     (mode->crop.width != width || mode->crop.height != height)))
>  		return NULL;
>  
> -	/* Check to see if the current mode exceeds the max frame rate */
> -	if (ov5640_framerates[fr] > ov5640_framerates[mode->max_fps])
> +	/*
> +	 * Check to see if the current mode exceeds the max frame rate.
> +	 * Only DVP mode uses the frame rate set by s_frame_interval, MIPI
> +	 * mode controls framerate by setting blankings.
> +	 */
> +	if (!ov5640_is_mipi(sensor) &&
> +	    ov5640_framerates[fr] > ov5640_framerates[mode->max_fps])
>  		return NULL;
>  
>  	return mode;

-- 
Regards,

Laurent Pinchart