Re: [PATCH v4 10/14] media: ov772x: reconstruct s_frame_interval()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2018-05-04 5:29 GMT+09:00 jacopo mondi <jacopo@xxxxxxxxxx>:
> 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+renesas@xxxxxxxxxx>
>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>> Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Akinobu Mita <akinobu.mita@xxxxxxxxx>
>> ---
>> * 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.

OK.

> 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.

OK.

>> +     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.

I would like to keep ov772x_select_fps() because the function name is
self descriptive and imples it doesn't change HW registers.  So I feel
it helps readability a bit.

> 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
>>



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux