diff mbox series

media: ov5640: report correct frame rate to user

Message ID 20221202104250.2634726-1-guoniu.zhou@oss.nxp.com (mailing list archive)
State Superseded
Delegated to: Sakari Ailus
Headers
Series media: ov5640: report correct frame rate to user |

Commit Message

G.N. Zhou (OSS) Dec. 2, 2022, 10:42 a.m. UTC 
  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
assumes the maximum frame rate supported by all modes is 60. But actully,
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(-)

Comments

Laurent Pinchart Dec. 2, 2022, 11:08 a.m. UTC | #1 
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;
>  	}
>
G.N. Zhou (OSS) Dec. 2, 2022, 12:51 p.m. UTC | #2 
Hi Laurent Pinchart,

Thank you for you replying and quick reviewing.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 2022年12月2日 19:08
> To: G.N. Zhou (OSS) <guoniu.zhou@oss.nxp.com>
> Cc: linux-media@vger.kernel.org; slongerbeam@gmail.com;
> mchehab@kernel.org; jacopo@jmondi.org; sakari.ailus@linux.intel.com
> Subject: Re: [PATCH] media: ov5640: report correct frame rate to user
> 
> 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
diff mbox series

Patch

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;
+
 	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;
 	}