On 22/01/18 11:59, Sakari Ailus wrote:
> Hi Hans,
>
> On Mon, Jan 22, 2018 at 11:33:28AM +0100, Hans Verkuil wrote:
>> On 22/01/18 11:26, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Mon, Jan 22, 2018 at 11:18:50AM +0100, Hans Verkuil wrote:
>>>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>>
>>>> Convert all g/s_parm calls to g/s_frame_interval. This allows us
>>>> to remove the g/s_parm ops since those are a duplicate of
>>>> g/s_frame_interval.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>> ---
>>>> drivers/media/i2c/mt9v011.c | 29 +++++++++----------
>>>> drivers/media/i2c/ov6650.c | 33 ++++++++++------------
>>>> drivers/media/i2c/ov7670.c | 26 +++++++++--------
>>>> drivers/media/i2c/ov7740.c | 29 ++++++++-----------
>>>> drivers/media/i2c/tvp514x.c | 37 +++++++++++--------------
>>>> drivers/media/i2c/vs6624.c | 29 ++++++++++---------
>>>> drivers/media/platform/atmel/atmel-isc.c | 10 ++-----
>>>> drivers/media/platform/atmel/atmel-isi.c | 12 ++------
>>>> drivers/media/platform/blackfin/bfin_capture.c | 14 +++-------
>>>> drivers/media/platform/marvell-ccic/mcam-core.c | 12 ++++----
>>>> drivers/media/platform/soc_camera/soc_camera.c | 10 ++++---
>>>> drivers/media/platform/via-camera.c | 4 +--
>>>> drivers/media/usb/em28xx/em28xx-video.c | 36 ++++++++++++++++++++----
>>>> 13 files changed, 137 insertions(+), 144 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/mt9v011.c b/drivers/media/i2c/mt9v011.c
>>>> index 5e29064fae91..0e0bcc8b67ca 100644
>>>> --- a/drivers/media/i2c/mt9v011.c
>>>> +++ b/drivers/media/i2c/mt9v011.c
>>>> @@ -364,33 +364,30 @@ static int mt9v011_set_fmt(struct v4l2_subdev *sd,
>>>> return 0;
>>>> }
>>>>
>>>> -static int mt9v011_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
>>>> +static int mt9v011_g_frame_interval(struct v4l2_subdev *sd,
>>>> + struct v4l2_subdev_frame_interval *ival)
>>>> {
>>>> - struct v4l2_captureparm *cp = &parms->parm.capture;
>>>> -
>>>> - if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>>> + if (ival->pad)
>>>> return -EINVAL;
>>>
>>> The pad number checks are already present in v4l2-subdev.c. Do you think
>>> we'll need them in drivers as well?
>>>
>>> It's true that another driver could mis-use this interface. In that case
>>> I'd introduce a wrapper to the op rather than introduce the check in every
>>> driver.
>>
>> I'm not that keen on introducing wrappers for an op. I wouldn't actually know
>> how to implement that cleanly. Since the pad check is subdev driver specific,
>> and the overhead of a wrapper is almost certainly higher than just doing this
>> check I feel it is OK to do this.
>
> For a majority of drivers, the check is that the pad must be a valid
> pad in an entity. The case where the overhead could matter (it's just a
> single if) is when called through an IOCTL from the user space. And the
> subdevices' IOCTL handler already performs this check. The wrapper would
> simply extend the check to other drivers calling the op.
>
> It's easy to miss such checks in drivers. We've had quite a few such cases
> in the past. Performing the check universally would make the framework more
> robust.
>
> What comes to this patchset, I'd omit the pad number check as other drivers
> don't do such checks either --- unless the check is more strict than what
> the interface allows.
I'll remove the check.
Hans
>
>>
>>>
>>>>
>>>> - memset(cp, 0, sizeof(struct v4l2_captureparm));
>>>> - cp->capability = V4L2_CAP_TIMEPERFRAME;
>>>> + memset(ival->reserved, 0, sizeof(ival->reserved));
>>>> calc_fps(sd,
>>>> - &cp->timeperframe.numerator,
>>>> - &cp->timeperframe.denominator);
>>>> + &ival->interval.numerator,
>>>> + &ival->interval.denominator);
>>>>
>>>> return 0;
>>>> }
>>
>> Regards,
>>
>> Hans
>>
>
