| Message ID | 20230627131830.54601-19-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov2680: Bugfixes + ACPI + selection(crop-tgt) API support | expand |
On 27/06/2023 15:18, Hans de Goede wrote: > Fix and simplify ov2680_enum_frame_interval(), the index is not > an index into ov2680_mode_data[], so using OV2680_MODE_MAX is wrong. > > Instead it is an index indexing the different framerates for > the resolution specified in fie->width, fie->height. > > Since the ov2680 code only supports a single fixed framerate, > index must always be 0 and we don't need to check the other > fie input values. But in this case the user could ask which single frame interval is supported for a frame size that is _not_ supported, and be told that the driver can give them 30fps. I think we still need to check the validity of the other inputs and return -EINVAL when they're invalid. > > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/i2c/ov2680.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c > index b011dadbb98a..7ca70877abf1 100644 > --- a/drivers/media/i2c/ov2680.c > +++ b/drivers/media/i2c/ov2680.c > @@ -532,17 +532,13 @@ static int ov2680_enum_frame_interval(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_frame_interval_enum *fie) > { > - struct v4l2_fract tpf; > + struct ov2680_dev *sensor = to_ov2680_dev(sd); > > - if (fie->index >= OV2680_MODE_MAX || fie->width > OV2680_WIDTH_MAX || > - fie->height > OV2680_HEIGHT_MAX || > - fie->which > V4L2_SUBDEV_FORMAT_ACTIVE) > + /* Only 1 framerate */ > + if (fie->index) > return -EINVAL; > > - tpf.denominator = OV2680_FRAME_RATE; > - tpf.numerator = 1; > - > - fie->interval = tpf; > + fie->interval = sensor->frame_interval; > > return 0; > }
Hi, On 7/3/23 09:26, Dan Scally wrote: > > On 27/06/2023 15:18, Hans de Goede wrote: >> Fix and simplify ov2680_enum_frame_interval(), the index is not >> an index into ov2680_mode_data[], so using OV2680_MODE_MAX is wrong. >> >> Instead it is an index indexing the different framerates for >> the resolution specified in fie->width, fie->height. >> >> Since the ov2680 code only supports a single fixed framerate, >> index must always be 0 and we don't need to check the other >> fie input values. > > > But in this case the user could ask which single frame interval is supported for a frame size that is _not_ supported, and be told that the driver can give them 30fps. I think we still need to check the validity of the other inputs and return -EINVAL when they're invalid. Ok for v4 I'll re-add the fie->which check and also add a check that the passed in width + height are a valid combination from ov2680_mode_data[]. Regards, Hans > >> >> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/media/i2c/ov2680.c | 12 ++++-------- >> 1 file changed, 4 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c >> index b011dadbb98a..7ca70877abf1 100644 >> --- a/drivers/media/i2c/ov2680.c >> +++ b/drivers/media/i2c/ov2680.c >> @@ -532,17 +532,13 @@ static int ov2680_enum_frame_interval(struct v4l2_subdev *sd, >> struct v4l2_subdev_state *sd_state, >> struct v4l2_subdev_frame_interval_enum *fie) >> { >> - struct v4l2_fract tpf; >> + struct ov2680_dev *sensor = to_ov2680_dev(sd); >> - if (fie->index >= OV2680_MODE_MAX || fie->width > OV2680_WIDTH_MAX || >> - fie->height > OV2680_HEIGHT_MAX || >> - fie->which > V4L2_SUBDEV_FORMAT_ACTIVE) >> + /* Only 1 framerate */ >> + if (fie->index) >> return -EINVAL; >> - tpf.denominator = OV2680_FRAME_RATE; >> - tpf.numerator = 1; >> - >> - fie->interval = tpf; >> + fie->interval = sensor->frame_interval; >> return 0; >> } >
diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c index b011dadbb98a..7ca70877abf1 100644 --- a/drivers/media/i2c/ov2680.c +++ b/drivers/media/i2c/ov2680.c @@ -532,17 +532,13 @@ static int ov2680_enum_frame_interval(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state, struct v4l2_subdev_frame_interval_enum *fie) { - struct v4l2_fract tpf; + struct ov2680_dev *sensor = to_ov2680_dev(sd); - if (fie->index >= OV2680_MODE_MAX || fie->width > OV2680_WIDTH_MAX || - fie->height > OV2680_HEIGHT_MAX || - fie->which > V4L2_SUBDEV_FORMAT_ACTIVE) + /* Only 1 framerate */ + if (fie->index) return -EINVAL; - tpf.denominator = OV2680_FRAME_RATE; - tpf.numerator = 1; - - fie->interval = tpf; + fie->interval = sensor->frame_interval; return 0; }