Re: [PATCH v9 07/11] media: i2c: ov772x: Support frame interval handling

[jacopo mondi]

Hi Hans,

On Wed, Feb 21, 2018 at 01:12:14PM +0100, Hans Verkuil wrote:

[snip]

> > +static int ov772x_g_frame_interval(struct v4l2_subdev *sd,
> > +				   struct v4l2_subdev_frame_interval *ival)
> > +{
> > +	struct ov772x_priv *priv = to_ov772x(sd);
> > +	struct v4l2_fract *tpf = &ival->interval;
> > +
> > +	memset(ival->reserved, 0, sizeof(ival->reserved));
>
> This memset...
>
> > +	tpf->numerator = 1;
> > +	tpf->denominator = priv->fps;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ov772x_s_frame_interval(struct v4l2_subdev *sd,
> > +				   struct v4l2_subdev_frame_interval *ival)
> > +{
> > +	struct ov772x_priv *priv = to_ov772x(sd);
> > +	struct v4l2_fract *tpf = &ival->interval;
> > +
> > +	memset(ival->reserved, 0, sizeof(ival->reserved));
>
> ... and this memset can be dropped. The core code will memset this for you.
>
>

I see! Ok, I'll drop them in v10

> > +
> > +	return ov772x_set_frame_rate(priv, tpf, priv->cfmt, priv->win);
> > +}
> > +
> >  static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> >  {
> >  	struct ov772x_priv *priv = container_of(ctrl->handler,
> > @@ -757,6 +905,7 @@ static int ov772x_set_params(struct ov772x_priv *priv,
> >  			     const struct ov772x_win_size *win)
> >  {
> >  	struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
> > +	struct v4l2_fract tpf;
> >  	int ret;
> >  	u8  val;
> >
> > @@ -885,6 +1034,13 @@ static int ov772x_set_params(struct ov772x_priv *priv,
> >  	if (ret < 0)
> >  		goto ov772x_set_fmt_error;
> >
> > +	/* COM4, CLKRC: Set pixel clock and framerate. */
> > +	tpf.numerator = 1;
> > +	tpf.denominator = priv->fps;
> > +	ret = ov772x_set_frame_rate(priv, &tpf, cfmt, win);
> > +	if (ret < 0)
> > +		goto ov772x_set_fmt_error;
> > +
> >  	/*
> >  	 * set COM8
> >  	 */
> > @@ -1043,6 +1199,24 @@ static const struct v4l2_subdev_core_ops ov772x_subdev_core_ops = {
> >  	.s_power	= ov772x_s_power,
> >  };
> >
> > +static int ov772x_enum_frame_interval(struct v4l2_subdev *sd,
> > +				      struct v4l2_subdev_pad_config *cfg,
> > +				      struct v4l2_subdev_frame_interval_enum *fie)
> > +{
> > +	if (fie->pad || fie->index >= OV772X_N_FRAME_INTERVALS)
> > +		return -EINVAL;
> > +
> > +	if (fie->width != VGA_WIDTH && fie->width != QVGA_WIDTH)
> > +		return -EINVAL;
> > +	if (fie->height != VGA_HEIGHT && fie->height != QVGA_HEIGHT)
> > +		return -EINVAL;
> > +
> > +	fie->interval.numerator = 1;
> > +	fie->interval.denominator = ov772x_frame_intervals[fie->index];
> > +
> > +	return 0;
> > +}
> > +
> >  static int ov772x_enum_mbus_code(struct v4l2_subdev *sd,
> >  		struct v4l2_subdev_pad_config *cfg,
> >  		struct v4l2_subdev_mbus_code_enum *code)
> > @@ -1055,14 +1229,17 @@ static int ov772x_enum_mbus_code(struct v4l2_subdev *sd,
> >  }
> >
> >  static const struct v4l2_subdev_video_ops ov772x_subdev_video_ops = {
> > -	.s_stream	= ov772x_s_stream,
> > +	.s_stream		= ov772x_s_stream,
> > +	.s_frame_interval	= ov772x_s_frame_interval,
> > +	.g_frame_interval	= ov772x_g_frame_interval,
> >  };
> >
> >  static const struct v4l2_subdev_pad_ops ov772x_subdev_pad_ops = {
> > -	.enum_mbus_code = ov772x_enum_mbus_code,
> > -	.get_selection	= ov772x_get_selection,
> > -	.get_fmt	= ov772x_get_fmt,
> > -	.set_fmt	= ov772x_set_fmt,
> > +	.enum_frame_interval	= ov772x_enum_frame_interval,
> > +	.enum_mbus_code		= ov772x_enum_mbus_code,
> > +	.get_selection		= ov772x_get_selection,
> > +	.get_fmt		= ov772x_get_fmt,
> > +	.set_fmt		= ov772x_set_fmt,
>
> Shouldn't these last four ops be added in the previous patch?
> They don't have anything to do with the frame interval support.
>

If you look closely you'll notice I have just re-aligned them, since I
was at there to add enum_frame_interval operation

> Anyway, after taking care of the memsets and these four ops you can add
> my:
>
> Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>

Thanks
   j