Re: [PATCH 1/2] v4l2 subdevs: replace get/set_crop by get/set_selection

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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