Hi Laurent Pinchart,
Thank you for you replying and quick reviewing.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: 2022年12月2日 19:08
> To: G.N. Zhou (OSS) <guoniu.zhou@xxxxxxxxxxx>
> Cc: linux-media@xxxxxxxxxxxxxxx; slongerbeam@xxxxxxxxx;
> mchehab@xxxxxxxxxx; jacopo@xxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx
> 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@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
