Re: [PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection
Hi Laurent, On Mon, Dec 08, 2014 at 02:17:11AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Wednesday 03 December 2014 13:06:00 Sakari Ailus wrote: > > On Tue, Dec 02, 2014 at 01:21:40PM +0100, Hans Verkuil wrote: > > > From: Hans Verkuil > > > > > > The crop and selection pad ops are duplicates. Replace all uses of > > > get/set_crop by get/set_selection. This will make it possible to drop > > > get/set_crop altogether. > > > > > > Signed-off-by: Hans Verkuil > > > Cc: Sylwester Nawrocki > > > Cc: Laurent Pinchart > > > Cc: Prabhakar Lad > > > Cc: Philipp Zabel > > > > For both: > > > > Acked-by: Sakari Ailus > > > > Another point I'd like to draw attention to are the reserved fields --- some > > drivers appear to zero them whereas some pay no attention. Shouldn't we > > check in the sub-device IOCTL handler that the user has zeroed them, or > > zero them for the user? I think this has probably been discussed before on > > V4L2. Both have their advantages, probably zeroing them in the framework > > would be the best option. What do you think? > > I think we should at least be consistent across drivers. Duplicating checks > across drivers being more error-prone it would likely be better to implement > them in core code. The question that remains to be answered is whether we can > consider that bridge drivers will correctly zero reserved fields when using > the API internally. If not, we'll need wrapper functions around subdev > operations to zero reserved fields, or possibly just to output a WARN_ON (as > a > bridge driver bug should be fixed instead of silently ignored). I'd simply check these fields in v4l2-subdev.c ioctl wrappers. There are over 300 instances of v4l2_subdev_call(). It's at least possible to audit them. I'd favour that over adding a wrapper to each op, and then paying attention to the topic in reviews. It's not only a matter of that interface. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection
Hi Sakari, On Wednesday 03 December 2014 13:06:00 Sakari Ailus wrote: > On Tue, Dec 02, 2014 at 01:21:40PM +0100, Hans Verkuil wrote: > > From: Hans Verkuil > > > > The crop and selection pad ops are duplicates. Replace all uses of > > get/set_crop by get/set_selection. This will make it possible to drop > > get/set_crop altogether. > > > > Signed-off-by: Hans Verkuil > > Cc: Sylwester Nawrocki > > Cc: Laurent Pinchart > > Cc: Prabhakar Lad > > Cc: Philipp Zabel > > For both: > > Acked-by: Sakari Ailus > > Another point I'd like to draw attention to are the reserved fields --- some > drivers appear to zero them whereas some pay no attention. Shouldn't we > check in the sub-device IOCTL handler that the user has zeroed them, or > zero them for the user? I think this has probably been discussed before on > V4L2. Both have their advantages, probably zeroing them in the framework > would be the best option. What do you think? I think we should at least be consistent across drivers. Duplicating checks across drivers being more error-prone it would likely be better to implement them in core code. The question that remains to be answered is whether we can consider that bridge drivers will correctly zero reserved fields when using the API internally. If not, we'll need wrapper functions around subdev operations to zero reserved fields, or possibly just to output a WARN_ON (as a bridge driver bug should be fixed instead of silently ignored). > Definitely out of scope of this set. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection
On 12/03/14 12:06, Sakari Ailus wrote: > Hi Hans, > > On Tue, Dec 02, 2014 at 01:21:40PM +0100, Hans Verkuil wrote: >> From: Hans Verkuil >> >> The crop and selection pad ops are duplicates. Replace all uses of >> get/set_crop >> by get/set_selection. This will make it possible to drop get/set_crop >> altogether. >> >> Signed-off-by: Hans Verkuil >> Cc: Sylwester Nawrocki >> Cc: Laurent Pinchart >> Cc: Prabhakar Lad >> Cc: Philipp Zabel > > For both: > > Acked-by: Sakari Ailus > > Another point I'd like to draw attention to are the reserved fields --- some > drivers appear to zero them whereas some pay no attention. Shouldn't we > check in the sub-device IOCTL handler that the user has zeroed them, or zero > them for the user? I think this has probably been discussed before on V4L2. > Both have their advantages, probably zeroing them in the framework would be > the best option. What do you think? If the framework can zero, then that's always better. Also note that valgrind understands the subdev ioctls, so that will be able to checks apps as well. I haven't really looked into this yet, but I'm happy to review patches :-) Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection
Hi Hans,
On Wednesday 03 December 2014 12:17:57 Hans Verkuil wrote:
> On 12/03/14 12:14, Sylwester Nawrocki wrote:
> > On 02/12/14 13:21, Hans Verkuil wrote:
> >> -static int s5k6aa_set_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> >> *fh,
> >> - struct v4l2_subdev_crop *crop)
> >> +static int s5k6aa_set_selection(struct v4l2_subdev *sd,
> >> + struct v4l2_subdev_fh *fh,
> >> + struct v4l2_subdev_selection *sel)
> >> {
> >>struct s5k6aa *s5k6aa = to_s5k6aa(sd);
> >>struct v4l2_mbus_framefmt *mf;
> >>unsigned int max_x, max_y;
> >>struct v4l2_rect *crop_r;
> >>
> >> + if (sel->pad || sel->target != V4L2_SEL_TGT_CROP)
> >> + return -EINVAL;
> >> +
> >
> > Isn't checking sel->pad redundant here ? There is already the pad index
> > validation in check_selection() in v4l2-subdev.c and this driver has only
> > one pad.
>
> If it is called from a bridge driver, then it hasn't gone through
> check_selection().
>
> That said, if it is called from a bridge driver, then one might expect
> correct usage of pad.
>
> Laurent, do you have an opinion on this?
I would expect the pad to be valid when called from a bridge driver. We could
double-check that in subdev drivers as a debugging help, but I'm not sure if
it's worth it.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection
On 12/03/14 12:49, Sylwester Nawrocki wrote:
> On 03/12/14 12:17, Hans Verkuil wrote:
>> Hi Sylwester,
>>
>> On 12/03/14 12:14, Sylwester Nawrocki wrote:
Hi Hans,
On 02/12/14 13:21, Hans Verkuil wrote:
>> -static int s5k6aa_set_crop(struct v4l2_subdev *sd, struct
>> v4l2_subdev_fh *fh,
>> - struct v4l2_subdev_crop *crop)
>> +static int s5k6aa_set_selection(struct v4l2_subdev *sd,
>> +struct v4l2_subdev_fh *fh,
>> +struct v4l2_subdev_selection *sel)
>> {
>> struct s5k6aa *s5k6aa = to_s5k6aa(sd);
>> struct v4l2_mbus_framefmt *mf;
>> unsigned int max_x, max_y;
>> struct v4l2_rect *crop_r;
>>
>> +if (sel->pad || sel->target != V4L2_SEL_TGT_CROP)
>> +return -EINVAL;
>> +
Isn't checking sel->pad redundant here ? There is already the pad index
validation in check_selection() in v4l2-subdev.c and this driver has only
one pad.
>>
>> If it is called from a bridge driver, then it hasn't gone through
>> check_selection().
>>
>> That said, if it is called from a bridge driver, then one might expect
>> correct usage of pad.
>
> Indeed, there is still a possibility to have wrong pad index passed
> to those functions. I won't object to this patch being merged as is,
> even though functional changes could be minimized by not adding a
> check which wasn't originally there. :)
>
> Acked-by: Sylwester Nawrocki
>
I've dropped the sel->pad check.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection
On 03/12/14 12:17, Hans Verkuil wrote:
> Hi Sylwester,
>
> On 12/03/14 12:14, Sylwester Nawrocki wrote:
>> > Hi Hans,
>> >
>> > On 02/12/14 13:21, Hans Verkuil wrote:
>>> >> -static int s5k6aa_set_crop(struct v4l2_subdev *sd, struct
>>> >> v4l2_subdev_fh *fh,
>>> >> - struct v4l2_subdev_crop *crop)
>>> >> +static int s5k6aa_set_selection(struct v4l2_subdev *sd,
>>> >> +struct v4l2_subdev_fh *fh,
>>> >> +struct v4l2_subdev_selection *sel)
>>> >> {
>>> >> struct s5k6aa *s5k6aa = to_s5k6aa(sd);
>>> >> struct v4l2_mbus_framefmt *mf;
>>> >> unsigned int max_x, max_y;
>>> >> struct v4l2_rect *crop_r;
>>> >>
>>> >> +if (sel->pad || sel->target != V4L2_SEL_TGT_CROP)
>>> >> +return -EINVAL;
>>> >> +
>> >
>> > Isn't checking sel->pad redundant here ? There is already the pad index
>> > validation in check_selection() in v4l2-subdev.c and this driver has only
>> > one pad.
>
> If it is called from a bridge driver, then it hasn't gone through
> check_selection().
>
> That said, if it is called from a bridge driver, then one might expect
> correct usage of pad.
Indeed, there is still a possibility to have wrong pad index passed
to those functions. I won't object to this patch being merged as is,
even though functional changes could be minimized by not adding a
check which wasn't originally there. :)
Acked-by: Sylwester Nawrocki
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection
Hi Sylwester,
On 12/03/14 12:14, Sylwester Nawrocki wrote:
> Hi Hans,
>
> On 02/12/14 13:21, Hans Verkuil wrote:
>> -static int s5k6aa_set_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh
>> *fh,
>> - struct v4l2_subdev_crop *crop)
>> +static int s5k6aa_set_selection(struct v4l2_subdev *sd,
>> +struct v4l2_subdev_fh *fh,
>> +struct v4l2_subdev_selection *sel)
>> {
>> struct s5k6aa *s5k6aa = to_s5k6aa(sd);
>> struct v4l2_mbus_framefmt *mf;
>> unsigned int max_x, max_y;
>> struct v4l2_rect *crop_r;
>>
>> +if (sel->pad || sel->target != V4L2_SEL_TGT_CROP)
>> +return -EINVAL;
>> +
>
> Isn't checking sel->pad redundant here ? There is already the pad index
> validation in check_selection() in v4l2-subdev.c and this driver has only
> one pad.
If it is called from a bridge driver, then it hasn't gone through
check_selection().
That said, if it is called from a bridge driver, then one might expect
correct usage of pad.
Laurent, do you have an opinion on this?
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection
On Wed, Dec 03, 2014 at 12:14:49PM +0100, Sylwester Nawrocki wrote:
> Hi Hans,
>
> On 02/12/14 13:21, Hans Verkuil wrote:
> > -static int s5k6aa_set_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> > *fh,
> > - struct v4l2_subdev_crop *crop)
> > +static int s5k6aa_set_selection(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_selection *sel)
> > {
> > struct s5k6aa *s5k6aa = to_s5k6aa(sd);
> > struct v4l2_mbus_framefmt *mf;
> > unsigned int max_x, max_y;
> > struct v4l2_rect *crop_r;
> >
> > + if (sel->pad || sel->target != V4L2_SEL_TGT_CROP)
> > + return -EINVAL;
> > +
>
> Isn't checking sel->pad redundant here ? There is already the pad index
> validation in check_selection() in v4l2-subdev.c and this driver has only
> one pad.
Good point. check_crop() does that for the [sg]_crop as well.
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection
Hi Hans,
On 02/12/14 13:21, Hans Verkuil wrote:
> -static int s5k6aa_set_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
> -struct v4l2_subdev_crop *crop)
> +static int s5k6aa_set_selection(struct v4l2_subdev *sd,
> + struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_selection *sel)
> {
> struct s5k6aa *s5k6aa = to_s5k6aa(sd);
> struct v4l2_mbus_framefmt *mf;
> unsigned int max_x, max_y;
> struct v4l2_rect *crop_r;
>
> + if (sel->pad || sel->target != V4L2_SEL_TGT_CROP)
> + return -EINVAL;
> +
Isn't checking sel->pad redundant here ? There is already the pad index
validation in check_selection() in v4l2-subdev.c and this driver has only
one pad.
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection
Hi Hans, On Tue, Dec 02, 2014 at 01:21:40PM +0100, Hans Verkuil wrote: > From: Hans Verkuil > > The crop and selection pad ops are duplicates. Replace all uses of > get/set_crop > by get/set_selection. This will make it possible to drop get/set_crop > altogether. > > Signed-off-by: Hans Verkuil > Cc: Sylwester Nawrocki > Cc: Laurent Pinchart > Cc: Prabhakar Lad > Cc: Philipp Zabel For both: Acked-by: Sakari Ailus Another point I'd like to draw attention to are the reserved fields --- some drivers appear to zero them whereas some pay no attention. Shouldn't we check in the sub-device IOCTL handler that the user has zeroed them, or zero them for the user? I think this has probably been discussed before on V4L2. Both have their advantages, probably zeroing them in the framework would be the best option. What do you think? Definitely out of scope of this set. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection
Hi Hans, Thanks for the patch. On Tue, Dec 2, 2014 at 12:21 PM, Hans Verkuil wrote: > From: Hans Verkuil > > The crop and selection pad ops are duplicates. Replace all uses of > get/set_crop > by get/set_selection. This will make it possible to drop get/set_crop > altogether. > > Signed-off-by: Hans Verkuil > Cc: Sylwester Nawrocki > Cc: Laurent Pinchart > Cc: Prabhakar Lad > Cc: Philipp Zabel > --- > drivers/media/i2c/mt9m032.c | 40 +++--- > drivers/media/i2c/mt9p031.c | 40 +++--- > drivers/media/i2c/mt9t001.c | 41 --- > drivers/media/i2c/mt9v032.c | 43 --- > drivers/media/i2c/s5k6aa.c | 44 +--- > drivers/staging/media/davinci_vpfe/dm365_isif.c | 69 > + Acked-by: Lad, Prabhakar Thanks, --Prabhakar Lad -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection
Hi Hans,
Thank you for the patch.
On Tuesday 02 December 2014 13:21:40 Hans Verkuil wrote:
> From: Hans Verkuil
>
> The crop and selection pad ops are duplicates. Replace all uses of
> get/set_crop by get/set_selection. This will make it possible to drop
> get/set_crop altogether.
>
> Signed-off-by: Hans Verkuil
> Cc: Sylwester Nawrocki
> Cc: Laurent Pinchart
> Cc: Prabhakar Lad
> Cc: Philipp Zabel
> ---
> drivers/media/i2c/mt9m032.c | 40 +++---
> drivers/media/i2c/mt9p031.c | 40 +++---
> drivers/media/i2c/mt9t001.c | 41 ---
> drivers/media/i2c/mt9v032.c | 43 ---
> drivers/media/i2c/s5k6aa.c | 44 +---
> drivers/staging/media/davinci_vpfe/dm365_isif.c | 69 +++---
> 6 files changed, 153 insertions(+), 124 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9m032.c b/drivers/media/i2c/mt9m032.c
> index 45b3fca..7b81eab 100644
> --- a/drivers/media/i2c/mt9m032.c
> +++ b/drivers/media/i2c/mt9m032.c
> @@ -422,22 +422,24 @@ done:
> return ret;
> }
>
> -static int mt9m032_get_pad_crop(struct v4l2_subdev *subdev,
> - struct v4l2_subdev_fh *fh,
> - struct v4l2_subdev_crop *crop)
> +static int mt9m032_get_pad_selection(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_selection *sel)
> {
> struct mt9m032 *sensor = to_mt9m032(subdev);
>
> + if (sel->pad || sel->target != V4L2_SEL_TGT_CROP)
> + return -EINVAL;
Nitpicking, could you please add a blank line here ? Same for similar
locations below in the Aptina sensors drivers.
> mutex_lock(&sensor->lock);
> - crop->rect = *__mt9m032_get_pad_crop(sensor, fh, crop->which);
> + sel->r = *__mt9m032_get_pad_crop(sensor, fh, sel->which);
> mutex_unlock(&sensor->lock);
>
> return 0;
> }
[snip]
> diff --git a/drivers/media/i2c/mt9p031.c b/drivers/media/i2c/mt9p031.c
> index edb76bd..b613456 100644
> --- a/drivers/media/i2c/mt9p031.c
> +++ b/drivers/media/i2c/mt9p031.c
> @@ -581,37 +581,41 @@ static int mt9p031_set_format(struct v4l2_subdev
> *subdev, return 0;
> }
>
> -static int mt9p031_get_crop(struct v4l2_subdev *subdev,
> - struct v4l2_subdev_fh *fh,
> - struct v4l2_subdev_crop *crop)
> +static int mt9p031_get_selection(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_selection *sel)
> {
> struct mt9p031 *mt9p031 = to_mt9p031(subdev);
>
> - crop->rect = *__mt9p031_get_pad_crop(mt9p031, fh, crop->pad,
> - crop->which);
> + if (sel->pad || sel->target != V4L2_SEL_TGT_CROP)
> + return -EINVAL;
> + sel->r = *__mt9p031_get_pad_crop(mt9p031, fh, sel->pad,
> + sel->which);
And a bit more nitpicking, could you please keep the sel->which alignment with
mt9p031 ?
> return 0;
> }
[snip]
> diff --git a/drivers/media/i2c/mt9t001.c b/drivers/media/i2c/mt9t001.c
> index d9e9889..2a907a9 100644
> --- a/drivers/media/i2c/mt9t001.c
> +++ b/drivers/media/i2c/mt9t001.c
> @@ -401,39 +401,44 @@ static int mt9t001_set_format(struct v4l2_subdev
> *subdev, return 0;
> }
>
> -static int mt9t001_get_crop(struct v4l2_subdev *subdev,
> - struct v4l2_subdev_fh *fh,
> - struct v4l2_subdev_crop *crop)
> +static int mt9t001_get_selection(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_selection *sel)
> {
> struct mt9t001 *mt9t001 = to_mt9t001(subdev);
>
> - crop->rect = *__mt9t001_get_pad_crop(mt9t001, fh, crop->pad,
> - crop->which);
> + if (sel->pad || sel->target != V4L2_SEL_TGT_CROP)
> + return -EINVAL;
> + sel->r = *__mt9t001_get_pad_crop(mt9t001, fh, sel->pad,
> + sel->which);
Ditto.
> return 0;
> }
[snip]
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index 93687c1..0d56b4e 100644
> --- a/drivers/media/i2c/mt9v032.c
> +++ b/drivers/media/i2c/mt9v032.c
> @@ -552,39 +552,44 @@ static int mt9v032_set_format(struct v4l2_subdev
> *subdev, return 0;
> }
>
> -static int mt9v032_get_crop(struct v4l2_subdev *subdev,
> - struct v4l2_subdev_fh *fh,
> - struct v4l2_subdev_crop *crop)
> +static int mt9v032_get_selection(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_selection *sel)
> {
> struct mt9v032 *mt9v032 = to_mt
[PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection
From: Hans Verkuil
The crop and selection pad ops are duplicates. Replace all uses of get/set_crop
by get/set_selection. This will make it possible to drop get/set_crop
altogether.
Signed-off-by: Hans Verkuil
Cc: Sylwester Nawrocki
Cc: Laurent Pinchart
Cc: Prabhakar Lad
Cc: Philipp Zabel
---
drivers/media/i2c/mt9m032.c | 40 +++---
drivers/media/i2c/mt9p031.c | 40 +++---
drivers/media/i2c/mt9t001.c | 41 ---
drivers/media/i2c/mt9v032.c | 43 ---
drivers/media/i2c/s5k6aa.c | 44 +---
drivers/staging/media/davinci_vpfe/dm365_isif.c | 69 +
6 files changed, 153 insertions(+), 124 deletions(-)
diff --git a/drivers/media/i2c/mt9m032.c b/drivers/media/i2c/mt9m032.c
index 45b3fca..7b81eab 100644
--- a/drivers/media/i2c/mt9m032.c
+++ b/drivers/media/i2c/mt9m032.c
@@ -422,22 +422,24 @@ done:
return ret;
}
-static int mt9m032_get_pad_crop(struct v4l2_subdev *subdev,
- struct v4l2_subdev_fh *fh,
- struct v4l2_subdev_crop *crop)
+static int mt9m032_get_pad_selection(struct v4l2_subdev *subdev,
+struct v4l2_subdev_fh *fh,
+struct v4l2_subdev_selection *sel)
{
struct mt9m032 *sensor = to_mt9m032(subdev);
+ if (sel->pad || sel->target != V4L2_SEL_TGT_CROP)
+ return -EINVAL;
mutex_lock(&sensor->lock);
- crop->rect = *__mt9m032_get_pad_crop(sensor, fh, crop->which);
+ sel->r = *__mt9m032_get_pad_crop(sensor, fh, sel->which);
mutex_unlock(&sensor->lock);
return 0;
}
-static int mt9m032_set_pad_crop(struct v4l2_subdev *subdev,
- struct v4l2_subdev_fh *fh,
- struct v4l2_subdev_crop *crop)
+static int mt9m032_set_pad_selection(struct v4l2_subdev *subdev,
+struct v4l2_subdev_fh *fh,
+struct v4l2_subdev_selection *sel)
{
struct mt9m032 *sensor = to_mt9m032(subdev);
struct v4l2_mbus_framefmt *format;
@@ -445,9 +447,11 @@ static int mt9m032_set_pad_crop(struct v4l2_subdev *subdev,
struct v4l2_rect rect;
int ret = 0;
+ if (sel->pad || sel->target != V4L2_SEL_TGT_CROP)
+ return -EINVAL;
mutex_lock(&sensor->lock);
- if (sensor->streaming && crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+ if (sensor->streaming && sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
ret = -EBUSY;
goto done;
}
@@ -455,13 +459,13 @@ static int mt9m032_set_pad_crop(struct v4l2_subdev
*subdev,
/* Clamp the crop rectangle boundaries and align them to a multiple of 2
* pixels to ensure a GRBG Bayer pattern.
*/
- rect.left = clamp(ALIGN(crop->rect.left, 2), MT9M032_COLUMN_START_MIN,
+ rect.left = clamp(ALIGN(sel->r.left, 2), MT9M032_COLUMN_START_MIN,
MT9M032_COLUMN_START_MAX);
- rect.top = clamp(ALIGN(crop->rect.top, 2), MT9M032_ROW_START_MIN,
+ rect.top = clamp(ALIGN(sel->r.top, 2), MT9M032_ROW_START_MIN,
MT9M032_ROW_START_MAX);
- rect.width = clamp_t(unsigned int, ALIGN(crop->rect.width, 2),
+ rect.width = clamp_t(unsigned int, ALIGN(sel->r.width, 2),
MT9M032_COLUMN_SIZE_MIN, MT9M032_COLUMN_SIZE_MAX);
- rect.height = clamp_t(unsigned int, ALIGN(crop->rect.height, 2),
+ rect.height = clamp_t(unsigned int, ALIGN(sel->r.height, 2),
MT9M032_ROW_SIZE_MIN, MT9M032_ROW_SIZE_MAX);
rect.width = min_t(unsigned int, rect.width,
@@ -469,21 +473,21 @@ static int mt9m032_set_pad_crop(struct v4l2_subdev
*subdev,
rect.height = min_t(unsigned int, rect.height,
MT9M032_PIXEL_ARRAY_HEIGHT - rect.top);
- __crop = __mt9m032_get_pad_crop(sensor, fh, crop->which);
+ __crop = __mt9m032_get_pad_crop(sensor, fh, sel->which);
if (rect.width != __crop->width || rect.height != __crop->height) {
/* Reset the output image size if the crop rectangle size has
* been modified.
*/
- format = __mt9m032_get_pad_format(sensor, fh, crop->which);
+ format = __mt9m032_get_pad_format(sensor, fh, sel->which);
format->width = rect.width;
format->height = rect.height;
}
*__crop = rect;
- crop->rect = rect;
+ sel->r = rect;
- if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+ if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
ret = mt9m032_update_geom_timing(sensor);
done:
@@ -690,8 +694,8 @@ static const struct v4l2_subdev_pad_ops mt9m032_pad_ops = {
.en
