| Message ID | 20221202104250.2634726-1-guoniu.zhou@oss.nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov5640: report correct frame rate to user | expand |
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; > } >
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 --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; }