| Message ID | 20220131144444.129036-4-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov5640: Rework the clock tree programming for MIPI | expand |
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;
On Thu, Feb 03, 2022 at 12:38:12AM +0200, Laurent Pinchart wrote: > 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. yes, and mostly because DVP mode seems to work well and I didn't want to change the way subdev is operated for DVP users. I would be more than happy to remove frame_interval completely. > > > 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
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;
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. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- drivers/media/i2c/ov5640.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)