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@xxxxxxxxx>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> 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;
>> }
>
