Re: [PATCH v2 1/3] sur40: properly report a single frame rate of 60 FPS

[Hans Verkuil <hverkuil@xs4all.nl>]

On 07/05/16 09:06, Hans Verkuil wrote:
> On 07/05/2016 08:56 AM, Florian Echtler wrote:
>> Hello Hans,
>>
>> On 05.07.2016 08:41, Hans Verkuil wrote:
>>> On 05/31/2016 10:15 PM, Florian Echtler wrote:
>>>> The device hardware is always running at 60 FPS, so report this both via
>>>> PARM_IOCTL and ENUM_FRAMEINTERVALS.
>>>>
>>>> Signed-off-by: Martin Kaltenbrunner <modin@yuri.at>
>>>> Signed-off-by: Florian Echtler <floe@butterbrot.org>
>>>> ---
>>>>  drivers/input/touchscreen/sur40.c | 20 ++++++++++++++++++--
>>>>  1 file changed, 18 insertions(+), 2 deletions(-)
>>>>
>>>> @@ -880,6 +893,9 @@ static const struct v4l2_ioctl_ops sur40_video_ioctl_ops = {
>>>>  	.vidioc_enum_framesizes = sur40_vidioc_enum_framesizes,
>>>>  	.vidioc_enum_frameintervals = sur40_vidioc_enum_frameintervals,
>>>>  
>>>> +	.vidioc_g_parm = sur40_ioctl_parm,
>>>> +	.vidioc_s_parm = sur40_ioctl_parm,
>>>
>>> Why is s_parm added when you can't change the framerate?
>>
>> Oh, I thought it's mandatory to always have s_parm if you have g_parm
>> (even if it always returns the same values).
>>
>>> Same questions for the
>>> enum_frameintervals function: it doesn't hurt to have it, but if there is only
>>> one unchangeable framerate, then it doesn't make much sense.
>>
>> If you don't have enum_frameintervals, how would you find out about the
>> framerate otherwise? Is g_parm itself enough already for all userspace
>> tools?
> 
> It should be. The enum_frameintervals function is much newer than g_parm.
> 
> Frankly, I have the same problem with enum_framesizes: it reports only one
> size. These two enum ioctls are normally only implemented if there are at
> least two choices. If there is no choice, then G_FMT will return the size
> and G_PARM the framerate and there is no need to enumerate anything.
> 
> The problem is that the spec is ambiguous as to the requirements if there
> is only one choice for size and interval. Are the enum ioctls allowed in
> that case? Personally I think there is nothing against that. But should
> S_PARM also be allowed even though it can't actually change the frameperiod?
> 
> Don't bother making changes yet, let me think about this for a bit.

OK, I came to the conclusion that if enum_frameintervals returns one or
more intervals, then s_parm should be present, even if there is only one
possible interval.

I have updated the compliance utility to check for this.

Regards,

	Hans