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

2018-02-21 Thread Hans Verkuil 
On 02/21/18 16:16, jacopo mondi wrote:
>>>  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

Ah, sorry. I missed that. Never mind then :-)

Hans

> 
>> Anyway, after taking care of the memsets and these four ops you can add
>> my:
>>
>> Acked-by: Hans Verkuil 
> 
> Thanks
>j
>

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

2018-02-21 Thread Hans Verkuil 
On 02/21/18 16:16, jacopo mondi wrote:
>>>  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

Ah, sorry. I missed that. Never mind then :-)

Hans

> 
>> Anyway, after taking care of the memsets and these four ops you can add
>> my:
>>
>> Acked-by: Hans Verkuil 
> 
> Thanks
>j
>

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

2018-02-21 Thread 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 = >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 = >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(>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, , 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 

Thanks
   j

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

2018-02-21 Thread 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 = >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 = >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(>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, , 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 

Thanks
   j

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

2018-02-21 Thread Hans Verkuil 
On 02/19/18 17:59, Jacopo Mondi wrote:
> Add support to ov772x driver for frame intervals handling and enumeration.
> Tested with 10MHz and 24MHz input clock at VGA and QVGA resolutions for
> 10, 15 and 30 frame per second rates.
> 
> Signed-off-by: Jacopo Mondi 
> Reviewed-by: Laurent Pinchart 
> ---
>  drivers/media/i2c/ov772x.c | 212 
> +
>  1 file changed, 195 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 23106d7..eba71d9 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -250,6 +250,7 @@
>  #define AEC_1p2 0x10 /*  01: 1/2  window */
>  #define AEC_1p4 0x20 /*  10: 1/4  window */
>  #define AEC_2p3 0x30 /*  11: Low 2/3 window */
> +#define COM4_RESERVED   0x01 /* Reserved bit */
> 
>  /* COM5 */
>  #define AFR_ON_OFF  0x80 /* Auto frame rate control ON/OFF selection */
> @@ -267,6 +268,10 @@
>   /* AEC max step control */
>  #define AEC_NO_LIMIT0x01 /*   0 : AEC incease step has limit */
>   /*   1 : No limit to AEC increase step */
> +/* CLKRC */
> + /* Input clock divider register */
> +#define CLKRC_RESERVED  0x80 /* Reserved bit */
> +#define CLKRC_DIV(n)((n) - 1)
> 
>  /* COM7 */
>   /* SCCB Register Reset */
> @@ -373,6 +378,19 @@
>  #define VERSION(pid, ver) ((pid<<8)|(ver&0xFF))
> 
>  /*
> + * PLL multipliers
> + */
> +struct {
> + unsigned int mult;
> + u8 com4;
> +} ov772x_pll[] = {
> + { 1, PLL_BYPASS, },
> + { 4, PLL_4x, },
> + { 6, PLL_6x, },
> + { 8, PLL_8x, },
> +};
> +
> +/*
>   * struct
>   */
> 
> @@ -388,6 +406,7 @@ struct ov772x_color_format {
>  struct ov772x_win_size {
>   char *name;
>   unsigned char com7_bit;
> + unsigned int  sizeimage;
>   struct v4l2_rect  rect;
>  };
> 
> @@ -404,6 +423,7 @@ struct ov772x_priv {
>   unsigned shortflag_hflip:1;
>   /* band_filter = COM8[5] ? 256 - BDBASE : 0 */
>   unsigned shortband_filter;
> + unsigned int  fps;
>  };
> 
>  /*
> @@ -487,27 +507,35 @@ static const struct ov772x_color_format ov772x_cfmts[] 
> = {
> 
>  static const struct ov772x_win_size ov772x_win_sizes[] = {
>   {
> - .name = "VGA",
> - .com7_bit = SLCT_VGA,
> + .name   = "VGA",
> + .com7_bit   = SLCT_VGA,
> + .sizeimage  = 510 * 748,
>   .rect = {
> - .left = 140,
> - .top = 14,
> - .width = VGA_WIDTH,
> - .height = VGA_HEIGHT,
> + .left   = 140,
> + .top= 14,
> + .width  = VGA_WIDTH,
> + .height = VGA_HEIGHT,
>   },
>   }, {
> - .name = "QVGA",
> - .com7_bit = SLCT_QVGA,
> + .name   = "QVGA",
> + .com7_bit   = SLCT_QVGA,
> + .sizeimage  = 278 * 576,
>   .rect = {
> - .left = 252,
> - .top = 6,
> - .width = QVGA_WIDTH,
> - .height = QVGA_HEIGHT,
> + .left   = 252,
> + .top= 6,
> + .width  = QVGA_WIDTH,
> + .height = QVGA_HEIGHT,
>   },
>   },
>  };
> 
>  /*
> + * frame rate settings lists
> + */
> +unsigned int ov772x_frame_intervals[] = { 5, 10, 15, 20, 30, 60 };
> +#define OV772X_N_FRAME_INTERVALS ARRAY_SIZE(ov772x_frame_intervals)
> +
> +/*
>   * general function
>   */
> 
> @@ -574,6 +602,126 @@ 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)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(>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;
> + for (i = 0, idx = 0; i < OV772X_N_FRAME_INTERVALS; i++) {
> + diff =

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

2018-02-21 Thread Hans Verkuil 
On 02/19/18 17:59, Jacopo Mondi wrote:
> Add support to ov772x driver for frame intervals handling and enumeration.
> Tested with 10MHz and 24MHz input clock at VGA and QVGA resolutions for
> 10, 15 and 30 frame per second rates.
> 
> Signed-off-by: Jacopo Mondi 
> Reviewed-by: Laurent Pinchart 
> ---
>  drivers/media/i2c/ov772x.c | 212 
> +
>  1 file changed, 195 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 23106d7..eba71d9 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -250,6 +250,7 @@
>  #define AEC_1p2 0x10 /*  01: 1/2  window */
>  #define AEC_1p4 0x20 /*  10: 1/4  window */
>  #define AEC_2p3 0x30 /*  11: Low 2/3 window */
> +#define COM4_RESERVED   0x01 /* Reserved bit */
> 
>  /* COM5 */
>  #define AFR_ON_OFF  0x80 /* Auto frame rate control ON/OFF selection */
> @@ -267,6 +268,10 @@
>   /* AEC max step control */
>  #define AEC_NO_LIMIT0x01 /*   0 : AEC incease step has limit */
>   /*   1 : No limit to AEC increase step */
> +/* CLKRC */
> + /* Input clock divider register */
> +#define CLKRC_RESERVED  0x80 /* Reserved bit */
> +#define CLKRC_DIV(n)((n) - 1)
> 
>  /* COM7 */
>   /* SCCB Register Reset */
> @@ -373,6 +378,19 @@
>  #define VERSION(pid, ver) ((pid<<8)|(ver&0xFF))
> 
>  /*
> + * PLL multipliers
> + */
> +struct {
> + unsigned int mult;
> + u8 com4;
> +} ov772x_pll[] = {
> + { 1, PLL_BYPASS, },
> + { 4, PLL_4x, },
> + { 6, PLL_6x, },
> + { 8, PLL_8x, },
> +};
> +
> +/*
>   * struct
>   */
> 
> @@ -388,6 +406,7 @@ struct ov772x_color_format {
>  struct ov772x_win_size {
>   char *name;
>   unsigned char com7_bit;
> + unsigned int  sizeimage;
>   struct v4l2_rect  rect;
>  };
> 
> @@ -404,6 +423,7 @@ struct ov772x_priv {
>   unsigned shortflag_hflip:1;
>   /* band_filter = COM8[5] ? 256 - BDBASE : 0 */
>   unsigned shortband_filter;
> + unsigned int  fps;
>  };
> 
>  /*
> @@ -487,27 +507,35 @@ static const struct ov772x_color_format ov772x_cfmts[] 
> = {
> 
>  static const struct ov772x_win_size ov772x_win_sizes[] = {
>   {
> - .name = "VGA",
> - .com7_bit = SLCT_VGA,
> + .name   = "VGA",
> + .com7_bit   = SLCT_VGA,
> + .sizeimage  = 510 * 748,
>   .rect = {
> - .left = 140,
> - .top = 14,
> - .width = VGA_WIDTH,
> - .height = VGA_HEIGHT,
> + .left   = 140,
> + .top= 14,
> + .width  = VGA_WIDTH,
> + .height = VGA_HEIGHT,
>   },
>   }, {
> - .name = "QVGA",
> - .com7_bit = SLCT_QVGA,
> + .name   = "QVGA",
> + .com7_bit   = SLCT_QVGA,
> + .sizeimage  = 278 * 576,
>   .rect = {
> - .left = 252,
> - .top = 6,
> - .width = QVGA_WIDTH,
> - .height = QVGA_HEIGHT,
> + .left   = 252,
> + .top= 6,
> + .width  = QVGA_WIDTH,
> + .height = QVGA_HEIGHT,
>   },
>   },
>  };
> 
>  /*
> + * frame rate settings lists
> + */
> +unsigned int ov772x_frame_intervals[] = { 5, 10, 15, 20, 30, 60 };
> +#define OV772X_N_FRAME_INTERVALS ARRAY_SIZE(ov772x_frame_intervals)
> +
> +/*
>   * general function
>   */
> 
> @@ -574,6 +602,126 @@ 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)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(>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;
> + for (i = 0, idx = 0; i < OV772X_N_FRAME_INTERVALS; i++) {
> + diff = abs(fps - ov772x_frame_intervals[i]);
> + if (diff <

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

2018-02-19 Thread kbuild test robot 
Hi Jacopo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.16-rc2 next-20180220]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Jacopo-Mondi/Renesas-Capture-Engine-Unit-CEU-V4L2-driver/20180220-101027
base:   git://linuxtv.org/media_tree.git master
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/media/i2c/ov772x.c:386:3: sparse: symbol 'ov772x_pll' was not 
>> declared. Should it be
>> drivers/media/i2c/ov772x.c:535:14: sparse: symbol 'ov772x_frame_intervals' 
>> was not declared. Should it be
   drivers/media/i2c/ov772x.c: In function 'ov772x_set_frame_rate.isra.2':
   drivers/media/i2c/ov772x.c:643:7: warning: 'fsize' may be used uninitialized 
in this function
pclk = fps COPYING CREDITS Documentation Kbuild Kconfig LICENSES 
MAINTAINERS Makefile README arch block certs crypto drivers firmware fs include 
init ipc kernel lib mm net samples scripts security sound tools usr virt fsize;
~^

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

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

2018-02-19 Thread kbuild test robot 
Hi Jacopo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.16-rc2 next-20180220]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Jacopo-Mondi/Renesas-Capture-Engine-Unit-CEU-V4L2-driver/20180220-101027
base:   git://linuxtv.org/media_tree.git master
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/media/i2c/ov772x.c:386:3: sparse: symbol 'ov772x_pll' was not 
>> declared. Should it be
>> drivers/media/i2c/ov772x.c:535:14: sparse: symbol 'ov772x_frame_intervals' 
>> was not declared. Should it be
   drivers/media/i2c/ov772x.c: In function 'ov772x_set_frame_rate.isra.2':
   drivers/media/i2c/ov772x.c:643:7: warning: 'fsize' may be used uninitialized 
in this function
pclk = fps COPYING CREDITS Documentation Kbuild Kconfig LICENSES 
MAINTAINERS Makefile README arch block certs crypto drivers firmware fs include 
init ipc kernel lib mm net samples scripts security sound tools usr virt fsize;
~^

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

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

2018-02-19 Thread kbuild test robot 
Hi Jacopo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.16-rc2 next-20180220]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Jacopo-Mondi/Renesas-Capture-Engine-Unit-CEU-V4L2-driver/20180220-101027
base:   git://linuxtv.org/media_tree.git master
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/media/i2c/ov772x.c: In function 'ov772x_set_frame_rate.isra.2':
>> drivers/media/i2c/ov772x.c:643:7: warning: 'fsize' may be used uninitialized 
>> in this function [-Wmaybe-uninitialized]
 pclk = fps * fsize;
 ~^

vim +/fsize +643 drivers/media/i2c/ov772x.c

   604  
   605  static int ov772x_set_frame_rate(struct ov772x_priv *priv,
   606   struct v4l2_fract *tpf,
   607   const struct ov772x_color_format *cfmt,
   608   const struct ov772x_win_size *win)
   609  {
   610  struct i2c_client *client = v4l2_get_subdevdata(>subdev);
   611  unsigned long fin = clk_get_rate(priv->clk);
   612  unsigned int fps = tpf->numerator ?
   613 tpf->denominator / tpf->numerator :
   614 tpf->denominator;
   615  unsigned int best_diff;
   616  unsigned int fsize;
   617  unsigned int pclk;
   618  unsigned int diff;
   619  unsigned int idx;
   620  unsigned int i;
   621  u8 clkrc = 0;
   622  u8 com4 = 0;
   623  int ret;
   624  
   625  /* Approximate to the closest supported frame interval. */
   626  best_diff = ~0L;
   627  for (i = 0, idx = 0; i < OV772X_N_FRAME_INTERVALS; i++) {
   628  diff = abs(fps - ov772x_frame_intervals[i]);
   629  if (diff < best_diff) {
   630  idx = i;
   631  best_diff = diff;
   632  }
   633  }
   634  fps = ov772x_frame_intervals[idx];
   635  
   636  /* Use image size (with blankings) to calculate desired pixel 
clock. */
   637  if ((cfmt->com7 & OFMT_MASK) == OFMT_RGB ||
   638  (cfmt->com7 & OFMT_MASK) == OFMT_YUV)
   639  fsize = win->sizeimage * 2;
   640  else if ((cfmt->com7 & OFMT_MASK) == OFMT_BRAW)
   641  fsize = win->sizeimage;
   642  
 > 643  pclk = fps * fsize;
   644  
   645  /*
   646   * Pixel clock generation circuit is pretty simple:
   647   *
   648   * Fin -> [ / CLKRC_div] -> [ * PLL_mult] -> pclk
   649   *
   650   * Try to approximate the desired pixel clock testing all 
available
   651   * PLL multipliers (1x, 4x, 6x, 8x) and calculate corresponding
   652   * divisor with:
   653   *
   654   * div = PLL_mult * Fin / pclk
   655   *
   656   * and re-calculate the pixel clock using it:
   657   *
   658   * pclk = Fin * PLL_mult / CLKRC_div
   659   *
   660   * Choose the PLL_mult and CLKRC_div pair that gives a pixel 
clock
   661   * closer to the desired one.
   662   *
   663   * The desired pixel clock is calculated using a known frame 
size
   664   * (blanking included) and FPS.
   665   */
   666  best_diff = ~0L;
   667  for (i = 0; i < ARRAY_SIZE(ov772x_pll); i++) {
   668  unsigned int pll_mult = ov772x_pll[i].mult;
   669  unsigned int pll_out = pll_mult * fin;
   670  unsigned int t_pclk;
   671  unsigned int div;
   672  
   673  if (pll_out < pclk)
   674  continue;
   675  
   676  div = DIV_ROUND_CLOSEST(pll_out, pclk);
   677  t_pclk = DIV_ROUND_CLOSEST(fin * pll_mult, div);
   678  diff = abs(pclk - t_pclk);
   679  if (diff < best_diff) {
   680  best_diff = diff;
   681  clkrc = CLKRC_DIV(div);
   682  com4 = ov772x_pll[i].com4;
   683  }
   684  }
   685  
   686  ret = ov772x_write(client, COM4, com4 | COM4_RESERVED);
   687  if (ret < 0)
   688  return ret;
   689  
   690  ret = ov772x_write(client, CLKRC, clkrc | CLKRC_RESERVED);
   691  if (ret < 0)
   692

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

2018-02-19 Thread kbuild test robot 
Hi Jacopo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.16-rc2 next-20180220]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Jacopo-Mondi/Renesas-Capture-Engine-Unit-CEU-V4L2-driver/20180220-101027
base:   git://linuxtv.org/media_tree.git master
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/media/i2c/ov772x.c: In function 'ov772x_set_frame_rate.isra.2':
>> drivers/media/i2c/ov772x.c:643:7: warning: 'fsize' may be used uninitialized 
>> in this function [-Wmaybe-uninitialized]
 pclk = fps * fsize;
 ~^

vim +/fsize +643 drivers/media/i2c/ov772x.c

   604  
   605  static int ov772x_set_frame_rate(struct ov772x_priv *priv,
   606   struct v4l2_fract *tpf,
   607   const struct ov772x_color_format *cfmt,
   608   const struct ov772x_win_size *win)
   609  {
   610  struct i2c_client *client = v4l2_get_subdevdata(>subdev);
   611  unsigned long fin = clk_get_rate(priv->clk);
   612  unsigned int fps = tpf->numerator ?
   613 tpf->denominator / tpf->numerator :
   614 tpf->denominator;
   615  unsigned int best_diff;
   616  unsigned int fsize;
   617  unsigned int pclk;
   618  unsigned int diff;
   619  unsigned int idx;
   620  unsigned int i;
   621  u8 clkrc = 0;
   622  u8 com4 = 0;
   623  int ret;
   624  
   625  /* Approximate to the closest supported frame interval. */
   626  best_diff = ~0L;
   627  for (i = 0, idx = 0; i < OV772X_N_FRAME_INTERVALS; i++) {
   628  diff = abs(fps - ov772x_frame_intervals[i]);
   629  if (diff < best_diff) {
   630  idx = i;
   631  best_diff = diff;
   632  }
   633  }
   634  fps = ov772x_frame_intervals[idx];
   635  
   636  /* Use image size (with blankings) to calculate desired pixel 
clock. */
   637  if ((cfmt->com7 & OFMT_MASK) == OFMT_RGB ||
   638  (cfmt->com7 & OFMT_MASK) == OFMT_YUV)
   639  fsize = win->sizeimage * 2;
   640  else if ((cfmt->com7 & OFMT_MASK) == OFMT_BRAW)
   641  fsize = win->sizeimage;
   642  
 > 643  pclk = fps * fsize;
   644  
   645  /*
   646   * Pixel clock generation circuit is pretty simple:
   647   *
   648   * Fin -> [ / CLKRC_div] -> [ * PLL_mult] -> pclk
   649   *
   650   * Try to approximate the desired pixel clock testing all 
available
   651   * PLL multipliers (1x, 4x, 6x, 8x) and calculate corresponding
   652   * divisor with:
   653   *
   654   * div = PLL_mult * Fin / pclk
   655   *
   656   * and re-calculate the pixel clock using it:
   657   *
   658   * pclk = Fin * PLL_mult / CLKRC_div
   659   *
   660   * Choose the PLL_mult and CLKRC_div pair that gives a pixel 
clock
   661   * closer to the desired one.
   662   *
   663   * The desired pixel clock is calculated using a known frame 
size
   664   * (blanking included) and FPS.
   665   */
   666  best_diff = ~0L;
   667  for (i = 0; i < ARRAY_SIZE(ov772x_pll); i++) {
   668  unsigned int pll_mult = ov772x_pll[i].mult;
   669  unsigned int pll_out = pll_mult * fin;
   670  unsigned int t_pclk;
   671  unsigned int div;
   672  
   673  if (pll_out < pclk)
   674  continue;
   675  
   676  div = DIV_ROUND_CLOSEST(pll_out, pclk);
   677  t_pclk = DIV_ROUND_CLOSEST(fin * pll_mult, div);
   678  diff = abs(pclk - t_pclk);
   679  if (diff < best_diff) {
   680  best_diff = diff;
   681  clkrc = CLKRC_DIV(div);
   682  com4 = ov772x_pll[i].com4;
   683  }
   684  }
   685  
   686  ret = ov772x_write(client, COM4, com4 | COM4_RESERVED);
   687  if (ret < 0)
   688  return ret;
   689  
   690  ret = ov772x_write(client, CLKRC, clkrc | CLKRC_RESERVED);
   691  if (ret < 0)
   692

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

2018-02-19 Thread Jacopo Mondi 
Add support to ov772x driver for frame intervals handling and enumeration.
Tested with 10MHz and 24MHz input clock at VGA and QVGA resolutions for
10, 15 and 30 frame per second rates.

Signed-off-by: Jacopo Mondi 
Reviewed-by: Laurent Pinchart 
---
 drivers/media/i2c/ov772x.c | 212 +
 1 file changed, 195 insertions(+), 17 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 23106d7..eba71d9 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -250,6 +250,7 @@
 #define AEC_1p2 0x10   /*  01: 1/2  window */
 #define AEC_1p4 0x20   /*  10: 1/4  window */
 #define AEC_2p3 0x30   /*  11: Low 2/3 window */
+#define COM4_RESERVED   0x01   /* Reserved bit */

 /* COM5 */
 #define AFR_ON_OFF  0x80   /* Auto frame rate control ON/OFF selection */
@@ -267,6 +268,10 @@
/* AEC max step control */
 #define AEC_NO_LIMIT0x01   /*   0 : AEC incease step has limit */
/*   1 : No limit to AEC increase step */
+/* CLKRC */
+   /* Input clock divider register */
+#define CLKRC_RESERVED  0x80   /* Reserved bit */
+#define CLKRC_DIV(n)((n) - 1)

 /* COM7 */
/* SCCB Register Reset */
@@ -373,6 +378,19 @@
 #define VERSION(pid, ver) ((pid<<8)|(ver&0xFF))

 /*
+ * PLL multipliers
+ */
+struct {
+   unsigned int mult;
+   u8 com4;
+} ov772x_pll[] = {
+   { 1, PLL_BYPASS, },
+   { 4, PLL_4x, },
+   { 6, PLL_6x, },
+   { 8, PLL_8x, },
+};
+
+/*
  * struct
  */

@@ -388,6 +406,7 @@ struct ov772x_color_format {
 struct ov772x_win_size {
char *name;
unsigned char com7_bit;
+   unsigned int  sizeimage;
struct v4l2_rect  rect;
 };

@@ -404,6 +423,7 @@ struct ov772x_priv {
unsigned shortflag_hflip:1;
/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
unsigned shortband_filter;
+   unsigned int  fps;
 };

 /*
@@ -487,27 +507,35 @@ static const struct ov772x_color_format ov772x_cfmts[] = {

 static const struct ov772x_win_size ov772x_win_sizes[] = {
{
-   .name = "VGA",
-   .com7_bit = SLCT_VGA,
+   .name   = "VGA",
+   .com7_bit   = SLCT_VGA,
+   .sizeimage  = 510 * 748,
.rect = {
-   .left = 140,
-   .top = 14,
-   .width = VGA_WIDTH,
-   .height = VGA_HEIGHT,
+   .left   = 140,
+   .top= 14,
+   .width  = VGA_WIDTH,
+   .height = VGA_HEIGHT,
},
}, {
-   .name = "QVGA",
-   .com7_bit = SLCT_QVGA,
+   .name   = "QVGA",
+   .com7_bit   = SLCT_QVGA,
+   .sizeimage  = 278 * 576,
.rect = {
-   .left = 252,
-   .top = 6,
-   .width = QVGA_WIDTH,
-   .height = QVGA_HEIGHT,
+   .left   = 252,
+   .top= 6,
+   .width  = QVGA_WIDTH,
+   .height = QVGA_HEIGHT,
},
},
 };

 /*
+ * frame rate settings lists
+ */
+unsigned int ov772x_frame_intervals[] = { 5, 10, 15, 20, 30, 60 };
+#define OV772X_N_FRAME_INTERVALS ARRAY_SIZE(ov772x_frame_intervals)
+
+/*
  * general function
  */

@@ -574,6 +602,126 @@ 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)
+{
+   struct i2c_client *client = v4l2_get_subdevdata(>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;
+   for (i = 0, idx = 0; i < OV772X_N_FRAME_INTERVALS; i++) {
+   diff = abs(fps - ov772x_frame_intervals[i]);
+   if (diff < best_diff) {
+   idx = i;
+   best_diff = diff;
+   }
+   }
+

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

2018-02-19 Thread Jacopo Mondi 
Add support to ov772x driver for frame intervals handling and enumeration.
Tested with 10MHz and 24MHz input clock at VGA and QVGA resolutions for
10, 15 and 30 frame per second rates.

Signed-off-by: Jacopo Mondi 
Reviewed-by: Laurent Pinchart 
---
 drivers/media/i2c/ov772x.c | 212 +
 1 file changed, 195 insertions(+), 17 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 23106d7..eba71d9 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -250,6 +250,7 @@
 #define AEC_1p2 0x10   /*  01: 1/2  window */
 #define AEC_1p4 0x20   /*  10: 1/4  window */
 #define AEC_2p3 0x30   /*  11: Low 2/3 window */
+#define COM4_RESERVED   0x01   /* Reserved bit */

 /* COM5 */
 #define AFR_ON_OFF  0x80   /* Auto frame rate control ON/OFF selection */
@@ -267,6 +268,10 @@
/* AEC max step control */
 #define AEC_NO_LIMIT0x01   /*   0 : AEC incease step has limit */
/*   1 : No limit to AEC increase step */
+/* CLKRC */
+   /* Input clock divider register */
+#define CLKRC_RESERVED  0x80   /* Reserved bit */
+#define CLKRC_DIV(n)((n) - 1)

 /* COM7 */
/* SCCB Register Reset */
@@ -373,6 +378,19 @@
 #define VERSION(pid, ver) ((pid<<8)|(ver&0xFF))

 /*
+ * PLL multipliers
+ */
+struct {
+   unsigned int mult;
+   u8 com4;
+} ov772x_pll[] = {
+   { 1, PLL_BYPASS, },
+   { 4, PLL_4x, },
+   { 6, PLL_6x, },
+   { 8, PLL_8x, },
+};
+
+/*
  * struct
  */

@@ -388,6 +406,7 @@ struct ov772x_color_format {
 struct ov772x_win_size {
char *name;
unsigned char com7_bit;
+   unsigned int  sizeimage;
struct v4l2_rect  rect;
 };

@@ -404,6 +423,7 @@ struct ov772x_priv {
unsigned shortflag_hflip:1;
/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
unsigned shortband_filter;
+   unsigned int  fps;
 };

 /*
@@ -487,27 +507,35 @@ static const struct ov772x_color_format ov772x_cfmts[] = {

 static const struct ov772x_win_size ov772x_win_sizes[] = {
{
-   .name = "VGA",
-   .com7_bit = SLCT_VGA,
+   .name   = "VGA",
+   .com7_bit   = SLCT_VGA,
+   .sizeimage  = 510 * 748,
.rect = {
-   .left = 140,
-   .top = 14,
-   .width = VGA_WIDTH,
-   .height = VGA_HEIGHT,
+   .left   = 140,
+   .top= 14,
+   .width  = VGA_WIDTH,
+   .height = VGA_HEIGHT,
},
}, {
-   .name = "QVGA",
-   .com7_bit = SLCT_QVGA,
+   .name   = "QVGA",
+   .com7_bit   = SLCT_QVGA,
+   .sizeimage  = 278 * 576,
.rect = {
-   .left = 252,
-   .top = 6,
-   .width = QVGA_WIDTH,
-   .height = QVGA_HEIGHT,
+   .left   = 252,
+   .top= 6,
+   .width  = QVGA_WIDTH,
+   .height = QVGA_HEIGHT,
},
},
 };

 /*
+ * frame rate settings lists
+ */
+unsigned int ov772x_frame_intervals[] = { 5, 10, 15, 20, 30, 60 };
+#define OV772X_N_FRAME_INTERVALS ARRAY_SIZE(ov772x_frame_intervals)
+
+/*
  * general function
  */

@@ -574,6 +602,126 @@ 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)
+{
+   struct i2c_client *client = v4l2_get_subdevdata(>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;
+   for (i = 0, idx = 0; i < OV772X_N_FRAME_INTERVALS; i++) {
+   diff = abs(fps - ov772x_frame_intervals[i]);
+   if (diff < best_diff) {
+   idx = i;
+   best_diff = diff;
+   }
+   }
+   fps = ov772x_frame_intervals[idx];
+
+   /* Use