devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: jacopo mondi <jacopo-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
To: Laurent Pinchart
	<laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
Cc: Jacopo Mondi
	<jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>,
	magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
	geert-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org,
	hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org,
	mchehab-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
	sakari.ailus-X3B1VOXEql0@public.gmane.org,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	mark.rutland-5wv7dgnIgG8@public.gmane.org,
	pombredanne-od1rfyK75/E@public.gmane.org,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-sh-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH v7 07/11] media: i2c: ov772x: Support frame interval handling
Date: Tue, 30 Jan 2018 10:47:51 +0100	[thread overview]
Message-ID: <20180130092808.GA11063@w540> (raw)
In-Reply-To: <1735356.2kmgrjUaxx@avalon>

Hi Laurent,

On Mon, Jan 29, 2018 at 01:01:01PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Friday, 26 January 2018 15:55:26 EET 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 <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
> > ---
> > drivers/media/i2c/ov772x.c | 315 +++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 310 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> > index 912b1b9..6d46748 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 value */
>
> I'd write "Reserved bits", "Reserved value" makes it sound like it's the value
> of the full register.
>

Ack

> >  /* COM5 */
> >  #define AFR_ON_OFF      0x80	/* Auto frame rate control ON/OFF selection
> */
> > @@ -267,6 +268,19 @@
> >  				/* AEC max step control */
> >  #define AEC_NO_LIMIT    0x01	/*   0 : AEC incease step has limit */
> >  				/*   1 : No limit to AEC increase step */
> > +/* CLKRC */
> > +				/* Input clock divider register */
> > +#define CLKRC_RESERVED  0x80	/* Reserved value */
> > +#define CLKRC_BYPASS    0x40	/* Bypass input clock divider */
> > +#define CLKRC_DIV2      0x01	/* Divide input clock by 2 */
> > +#define CLKRC_DIV3      0x02	/* Divide input clock by 3 */
> > +#define CLKRC_DIV4      0x03	/* Divide input clock by 4 */
> > +#define CLKRC_DIV5      0x04	/* Divide input clock by 5 */
> > +#define CLKRC_DIV6      0x05	/* Divide input clock by 6 */
> > +#define CLKRC_DIV8      0x07	/* Divide input clock by 8 */
> > +#define CLKRC_DIV10     0x09	/* Divide input clock by 10 */
> > +#define CLKRC_DIV16     0x0f	/* Divide input clock by 16 */
> > +#define CLKRC_DIV20     0x13	/* Divide input clock by 20 */
>
> How about just
>
> #define CLKRC_DIV(n)		((n) - 1)
>

Ack again,

> >  /* COM7 */
> >  				/* SCCB Register Reset */
> > @@ -373,6 +387,12 @@
> >  #define VERSION(pid, ver) ((pid<<8)|(ver&0xFF))
> >
> >  /*
> > + * Input clock frequencies
> > + */
> > +enum { OV772X_FIN_10MHz, OV772X_FIN_24MHz, OV772X_FIN_48MHz, OV772X_FIN_N,
> > };
> > +static unsigned int ov772x_fin_vals[] = { 10000000, 24000000, 48000000
> > };
> > +
> > +/*
> >   * struct
> >   */
> >
> > @@ -391,6 +411,16 @@ struct ov772x_win_size {
> >  	struct v4l2_rect	  rect;
> >  };
> >
> > +struct ov772x_pclk_config {
> > +	u8 com4;
> > +	u8 clkrc;
> > +};
> > +
> > +struct ov772x_frame_rate {
> > +	unsigned int fps;
> > +	const struct ov772x_pclk_config pclk[OV772X_FIN_N];
> > +};
> > +
> >  struct ov772x_priv {
> >  	struct v4l2_subdev                subdev;
> >  	struct v4l2_ctrl_handler	  hdl;
> > @@ -404,6 +434,7 @@ struct ov772x_priv {
> >  	unsigned short                    flag_hflip:1;
> >  	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
> >  	unsigned short                    band_filter;
> > +	unsigned int			  fps;
> >  };
> >
> >  /*
> > @@ -508,6 +539,154 @@ static const struct ov772x_win_size ov772x_win_sizes[]
> > = { };
> >
> >  /*
> > + * frame rate settings lists
> > + */
> > +unsigned int ov772x_frame_intervals[] = {10, 15, 30, 60};
> > +#define OV772X_N_FRAME_INTERVALS ARRAY_SIZE(ov772x_frame_intervals)
> > +
> > +static const struct ov772x_frame_rate vga_frame_rates[] = {
> > +	{	/* PCLK = 7,5 MHz */
> > +		.fps		= 10,
> > +		.pclk = {
> > +			[OV772X_FIN_10MHz] = {
> > +				.com4	= PLL_6x | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV8 | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_24MHz] = {
> > +				.com4	= PLL_BYPASS | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV3 | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_48MHz] = {
> > +				.com4	= PLL_BYPASS | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV6 | CLKRC_RESERVED,
> > +			},
> > +		},
> > +	},
> > +	{	/* PCLK = 12 MHz */
> > +		.fps		= 15,
> > +		.pclk = {
> > +			[OV772X_FIN_10MHz]	= {
> > +				.com4	= PLL_4x | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV3 | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_24MHz]	= {
> > +				.com4	= PLL_BYPASS | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV2 | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_48MHz]	= {
> > +				.com4	= PLL_BYPASS | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV4 | CLKRC_RESERVED,
> > +			},
> > +		},
> > +	},
> > +	{	/* PCLK = 24 MHz */
> > +		.fps		= 30,
> > +		.pclk = {
> > +			[OV772X_FIN_10MHz]	= {
> > +				.com4	= PLL_8x | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV3 | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_24MHz]	= {
> > +				.com4	= PLL_BYPASS | COM4_RESERVED,
> > +				.clkrc	= CLKRC_BYPASS | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_48MHz]	= {
> > +				.com4	= PLL_BYPASS | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV2 | CLKRC_RESERVED,
> > +			},
> > +		},
> > +	},
> > +	{	/* PCLK = 48 MHz */
> > +		.fps		= 60,
> > +		.pclk = {
> > +			[OV772X_FIN_10MHz]	= {
> > +				.com4	= PLL_8x | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV2 | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_24MHz]	= {
> > +				.com4	= PLL_4x | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV2 | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_48MHz]	= {
> > +				.com4	= PLL_BYPASS | COM4_RESERVED,
> > +				.clkrc	= CLKRC_BYPASS | CLKRC_RESERVED,
> > +			},
> > +		},
> > +	},
> > +};
> > +
> > +static const struct ov772x_frame_rate qvga_frame_rates[] = {
> > +	{	/* PCLK = 3,2 MHz */
> > +		.fps		= 10,
> > +		.pclk = {
> > +			[OV772X_FIN_10MHz] = {
> > +				.com4	= PLL_6x | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV16 | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_24MHz] = {
> > +				.com4	= PLL_BYPASS | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV8 | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_48MHz] = {
> > +				.com4	= PLL_BYPASS | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV16 | CLKRC_RESERVED,
> > +			},
> > +		},
> > +	},
> > +	{	/* PCLK = 4,8 MHz */
> > +		.fps		= 15,
> > +		.pclk = {
> > +			[OV772X_FIN_10MHz]	= {
> > +				.com4	= PLL_BYPASS | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV2 | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_24MHz]	= {
> > +				.com4	= PLL_BYPASS | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV5 | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_48MHz]	= {
> > +				.com4	= PLL_BYPASS | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV10 | CLKRC_RESERVED,
> > +			},
> > +		},
> > +	},
> > +	{	/* PCLK = 9,6 MHz */
> > +		.fps		= 30,
> > +		.pclk = {
> > +			[OV772X_FIN_10MHz]	= {
> > +				.com4	= PLL_BYPASS | COM4_RESERVED,
> > +				.clkrc	= CLKRC_BYPASS | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_24MHz]	= {
> > +				.com4	= PLL_4x | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV10 | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_48MHz]	= {
> > +				.com4	= PLL_4x | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV20 | CLKRC_RESERVED,
> > +			},
> > +		},
> > +	},
> > +	{	/* PCLK = 19 MHz */
> > +		.fps		= 60,
> > +		.pclk = {
> > +			[OV772X_FIN_10MHz]	= {
> > +				.com4	= PLL_4x | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV2 | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_24MHz]	= {
> > +				.com4	= PLL_6x | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV8 | CLKRC_RESERVED,
> > +			},
> > +			[OV772X_FIN_48MHz]	= {
> > +				.com4	= PLL_6x | COM4_RESERVED,
> > +				.clkrc	= CLKRC_DIV16 | CLKRC_RESERVED,
> > +			},
> > +		},
> > +	},
> > +};
> > +
> > +/*
> >   * general function
> >   */
>
> I'm afraid I'll have to ask the obvious: could we replace this table with
> dynamic computation ? You might be able to reuse the (probably badly named)
> aptina-pll library from drivers/media/i2c/
>

Mmmm, okay... I might be able to use the above mentioned library,
but that's designed for a still simple but more complex PLL with 2
dividers and one multiplier. I know I can model it to work on ov7720
PLL using limits, but since I only have 4 possible PLL multipliers
(1x, 2x, 4x, 8x) and a single divider it is simpler to test all 4 of
them and see which one approximate the desired pixel clock.

I will send v8 with this changed shortly.

Thanks
   j

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  reply	other threads:[~2018-01-30  9:47 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-26 13:55 [PATCH v7 00/11] Renesas Capture Engine Unit (CEU) V4L2 driver Jacopo Mondi
2018-01-26 13:55 ` [PATCH v7 01/11] dt-bindings: media: Add Renesas CEU bindings Jacopo Mondi
2018-01-26 13:55 ` [PATCH v7 02/11] include: media: Add Renesas CEU driver interface Jacopo Mondi
2018-01-26 13:55 ` [PATCH v7 03/11] media: platform: Add Renesas CEU driver Jacopo Mondi
2018-01-26 13:55 ` [PATCH v7 04/11] ARM: dts: r7s72100: Add Capture Engine Unit (CEU) Jacopo Mondi
2018-01-26 13:55 ` [PATCH v7 05/11] media: i2c: Copy ov772x soc_camera sensor driver Jacopo Mondi
2018-01-26 13:55 ` [PATCH v7 06/11] media: i2c: ov772x: Remove soc_camera dependencies Jacopo Mondi
2018-01-26 13:55 ` [PATCH v7 07/11] media: i2c: ov772x: Support frame interval handling Jacopo Mondi
2018-01-29 11:01   ` Laurent Pinchart
2018-01-30  9:47     ` jacopo mondi [this message]
2018-01-26 13:55 ` [PATCH v7 08/11] media: i2c: Copy tw9910 soc_camera sensor driver Jacopo Mondi
2018-01-26 13:55 ` [PATCH v7 09/11] media: i2c: tw9910: Remove soc_camera dependencies Jacopo Mondi
2018-01-26 13:55 ` [PATCH v7 10/11] arch: sh: migor: Use new renesas-ceu camera driver Jacopo Mondi
2018-01-26 13:55 ` [PATCH v7 11/11] media: i2c: ov7670: Fully set mbus frame fmt Jacopo Mondi
  -- strict thread matches above, loose matches on Subject: below --
2018-01-26 13:48 [PATCH v7 00/11] Renesas Capture Engine Unit (CEU) V4L2 driver Jacopo Mondi
2018-01-26 13:48 ` [PATCH v7 07/11] media: i2c: ov772x: Support frame interval handling Jacopo Mondi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180130092808.GA11063@w540 \
    --to=jacopo-aw8dsiih9cednm+yrofe0a@public.gmane.org \
    --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=festevam-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
    --cc=geert-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org \
    --cc=hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org \
    --cc=jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org \
    --cc=laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-sh-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=magnus.damm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
    --cc=mark.rutland-5wv7dgnIgG8@public.gmane.org \
    --cc=mchehab-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
    --cc=pombredanne-od1rfyK75/E@public.gmane.org \
    --cc=robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
    --cc=sakari.ailus-X3B1VOXEql0@public.gmane.org \
    --subject='Re: [PATCH v7 07/11] media: i2c: ov772x: Support frame interval handling' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).