Hi Akinobu, thank you for the patch On Mon, Apr 30, 2018 at 02:13:09AM +0900, Akinobu Mita wrote: > This splits the s_frame_interval() in subdev video ops into selecting the > frame interval and setting up the registers. > > This is a preparatory change to avoid accessing registers under power > saving mode. > > Cc: Jacopo Mondi <jacopo+rene...@jmondi.org> > Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > Cc: Hans Verkuil <hans.verk...@cisco.com> > Cc: Sakari Ailus <sakari.ai...@linux.intel.com> > Cc: Mauro Carvalho Chehab <mche...@s-opensource.com> > Signed-off-by: Akinobu Mita <akinobu.m...@gmail.com> > --- > * v4 > - No changes > > drivers/media/i2c/ov772x.c | 56 > +++++++++++++++++++++++++++++----------------- > 1 file changed, 35 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > index edc013d..7ea157e 100644 > --- a/drivers/media/i2c/ov772x.c > +++ b/drivers/media/i2c/ov772x.c > @@ -617,25 +617,16 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int > enable) > return 0; > } > > -static int ov772x_set_frame_rate(struct ov772x_priv *priv, > - struct v4l2_fract *tpf, > - const struct ov772x_color_format *cfmt, > - const struct ov772x_win_size *win) > +static unsigned int ov772x_select_fps(struct ov772x_priv *priv, > + struct v4l2_fract *tpf)
Please align to the open brace, as all the other functions in the
driver.
Also, is it worth making a function out of this? It is called from one
place only... see below.
> {
> - struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
> - unsigned long fin = clk_get_rate(priv->clk);
> unsigned int fps = tpf->numerator ?
> tpf->denominator / tpf->numerator :
> tpf->denominator;
> unsigned int best_diff;
> - unsigned int fsize;
> - unsigned int pclk;
> unsigned int diff;
> unsigned int idx;
> unsigned int i;
> - u8 clkrc = 0;
> - u8 com4 = 0;
> - int ret;
>
> /* Approximate to the closest supported frame interval. */
> best_diff = ~0L;
> @@ -646,7 +637,25 @@ static int ov772x_set_frame_rate(struct ov772x_priv
> *priv,
> best_diff = diff;
> }
> }
> - fps = ov772x_frame_intervals[idx];
> +
> + return ov772x_frame_intervals[idx];
> +}
> +
> +static int ov772x_set_frame_rate(struct ov772x_priv *priv,
> + unsigned int fps,
> + const struct ov772x_color_format *cfmt,
> + const struct ov772x_win_size *win)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&priv->subdev);
> + unsigned long fin = clk_get_rate(priv->clk);
> + unsigned int fsize;
> + unsigned int pclk;
> + unsigned int best_diff;
Please keep variable declarations sorted as in other functions in the
driver.
> + unsigned int diff;
> + unsigned int i;
> + u8 clkrc = 0;
> + u8 com4 = 0;
> + int ret;
>
> /* Use image size (with blankings) to calculate desired pixel clock. */
> switch (cfmt->com7 & OFMT_MASK) {
> @@ -711,10 +720,6 @@ static int ov772x_set_frame_rate(struct ov772x_priv
> *priv,
> if (ret < 0)
> return ret;
>
> - tpf->numerator = 1;
> - tpf->denominator = fps;
> - priv->fps = tpf->denominator;
> -
> return 0;
> }
>
> @@ -735,8 +740,20 @@ static int ov772x_s_frame_interval(struct v4l2_subdev
> *sd,
> {
> struct ov772x_priv *priv = to_ov772x(sd);
> struct v4l2_fract *tpf = &ival->interval;
> + unsigned int fps;
> + int ret;
> +
> + fps = ov772x_select_fps(priv, tpf);
That's the only caller of this function. I'm fine if you want to keep
it as it is though.
Thanks
j
> +
> + ret = ov772x_set_frame_rate(priv, fps, priv->cfmt, priv->win);
> + if (ret)
> + return ret;
>
> - return ov772x_set_frame_rate(priv, tpf, priv->cfmt, priv->win);
> + tpf->numerator = 1;
> + tpf->denominator = fps;
> + priv->fps = fps;
> +
> + return 0;
> }
>
> static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> @@ -993,7 +1010,6 @@ 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;
>
> @@ -1075,9 +1091,7 @@ static int ov772x_set_params(struct ov772x_priv *priv,
> 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);
> + ret = ov772x_set_frame_rate(priv, priv->fps, cfmt, win);
> if (ret < 0)
> goto ov772x_set_fmt_error;
>
> --
> 2.7.4
>
signature.asc
Description: PGP signature
