diff mbox series

[v3,18/29] media: ov2680: Fix ov2680_enum_frame_interval()

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

Commit Message

Hans de Goede June 27, 2023, 1:18 p.m. UTC 
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.

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(-)

Comments

Dan Scally July 3, 2023, 7:26 a.m. UTC | #1 
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;
>   }
Hans de Goede Aug. 1, 2023, 8:28 a.m. UTC | #2 
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 mbox series

Patch

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;
 }