Hi Hans,
On 03/07/20 11:12, Hans Verkuil wrote:
> On 03/07/2020 10:46, Luca Ceresoli wrote:
>> Hi Hans,
>>
>> [Cc: ing the imx274 maintainer]
>>
>> On 02/07/20 15:52, Hans Verkuil wrote:
>>> 1) the numerator and/or denominator might be 0, in that case
>>> fall back to the default frame interval. This is per the spec
>>> and this caused a v4l2-compliance failure.
>>>
>>> 2) the updated frame interval wasn't returned in the s_frame_interval
>>> subdev op.
>>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>>> ---
>>> drivers/media/i2c/imx274.c | 8 +++++---
>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>>> index 6011cec5e351..a9304b98f7af 100644
>>> --- a/drivers/media/i2c/imx274.c
>>> +++ b/drivers/media/i2c/imx274.c
>>> @@ -1235,6 +1235,8 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
>>> ret = imx274_set_frame_interval(imx274, fi->interval);
>>>
>>> if (!ret) {
>>> + fi->interval = imx274->frame_interval;
>>> +
>>> /*
>>> * exposure time range is decided by frame interval
>>> * need to update it after frame interval changes
>>> @@ -1730,9 +1732,9 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
>>> __func__, frame_interval.numerator,
>>> frame_interval.denominator);
>>>
>>> - if (frame_interval.numerator == 0) {
>>> - err = -EINVAL;
>>> - goto fail;
>>> + if (frame_interval.numerator == 0 || frame_interval.denominator) {
>>
>> Missing '== 0'?
>
> Oops!
>
>>
>> Please excuse my noobness, but I'm unable to understand which code path
>> would lead to calling imx274_set_frame_interval() with a zero here. I'm
>> assuming the v4l2 framework won't call imx274_s_frame_interval() with
>> numerator or denominator set to zero.
>>
>
> A bridge driver that is called with VIDIOC_S_PARM will just pass on the new
> interval to the sensor. And that interval can have either numerator and/or
> denominator set to 0 in which case the sensor driver should set the frame
> interval to a suitable default value (as per the spec). The bridge driver
> doesn't know what a suitable default value is, so this has to be done in the
> sensor driver.
Thanks for the explanation Hans. I was assuming the framework would
check sanity when passing calls between drivers, but now I checked the
v4l2_subdev_call() and it clearly does just pass the call through.
--
Luca
