diff mbox

media: i2c: add support for omnivision's ov2659 sensor

Message ID 1421365163-29394-1-git-send-email-prabhakar.csengg@gmail.com
State Superseded, archived
Headers show

Commit Message

Lad, Prabhakar Jan. 15, 2015, 11:39 p.m. UTC
From: Benoit Parrot <bparrot@ti.com>

this patch adds support for omnivision's ov2659
sensor.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
 .../devicetree/bindings/media/i2c/ov2659.txt       |   33 +
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 MAINTAINERS                                        |   10 +
 drivers/media/i2c/Kconfig                          |   11 +
 drivers/media/i2c/Makefile                         |    1 +
 drivers/media/i2c/ov2659.c                         | 1623 ++++++++++++++++++++
 include/media/ov2659.h                             |   33 +
 7 files changed, 1712 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2659.txt
 create mode 100644 drivers/media/i2c/ov2659.c
 create mode 100644 include/media/ov2659.h

Comments

Lad, Prabhakar Jan. 30, 2015, 3:46 p.m. UTC | #1
Hello,

On Thu, Jan 15, 2015 at 11:39 PM, Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> From: Benoit Parrot <bparrot@ti.com>
>
> this patch adds support for omnivision's ov2659
> sensor.
>
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> ---
>  .../devicetree/bindings/media/i2c/ov2659.txt       |   33 +
>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>  MAINTAINERS                                        |   10 +
>  drivers/media/i2c/Kconfig                          |   11 +
>  drivers/media/i2c/Makefile                         |    1 +
>  drivers/media/i2c/ov2659.c                         | 1623 ++++++++++++++++++++
>  include/media/ov2659.h                             |   33 +
>  7 files changed, 1712 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2659.txt
>  create mode 100644 drivers/media/i2c/ov2659.c
>  create mode 100644 include/media/ov2659.h
>
gentle ping for review comments.

Cheers,
--Prabhakar Lad
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Feb. 4, 2015, 2:55 p.m. UTC | #2
On Thu, Jan 15, 2015 at 5:39 PM, Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> From: Benoit Parrot <bparrot@ti.com>
>
> this patch adds support for omnivision's ov2659
> sensor.
>
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> ---

[...]

> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index b1df0ad..153cd92 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -118,6 +118,7 @@ nvidia      NVIDIA
>  nxp    NXP Semiconductors
>  onnn   ON Semiconductor Corp.
>  opencores      OpenCores.org
> +ovt    OmniVision Technologies

I'm surprised there are not already compatible strings with
OmniVision. There are some examples using "omnivision", but no dts
files and examples don't count.

The stock ticker is ovti, so please use that.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lad, Prabhakar Feb. 4, 2015, 3:01 p.m. UTC | #3
Hi Rob,

Thanks for the review.

On Wed, Feb 4, 2015 at 2:55 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Thu, Jan 15, 2015 at 5:39 PM, Lad, Prabhakar
> <prabhakar.csengg@gmail.com> wrote:
>> From: Benoit Parrot <bparrot@ti.com>
>>
>> this patch adds support for omnivision's ov2659
>> sensor.
>>
>> Signed-off-by: Benoit Parrot <bparrot@ti.com>
>> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>> ---
>
> [...]
>
>> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
>> index b1df0ad..153cd92 100644
>> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
>> @@ -118,6 +118,7 @@ nvidia      NVIDIA
>>  nxp    NXP Semiconductors
>>  onnn   ON Semiconductor Corp.
>>  opencores      OpenCores.org
>> +ovt    OmniVision Technologies
>
> I'm surprised there are not already compatible strings with
> OmniVision. There are some examples using "omnivision", but no dts
> files and examples don't count.
>
> The stock ticker is ovti, so please use that.
>
OK will update in the v2 version, I went with website which was ovt [1].

[1] http://www.ovt.com/

Regards,
--Prabhakar Lad
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Fabio Estevam Feb. 4, 2015, 3:23 p.m. UTC | #4
Rob,

On Wed, Feb 4, 2015 at 12:55 PM, Rob Herring <robherring2@gmail.com> wrote:

> I'm surprised there are not already compatible strings with
> OmniVision. There are some examples using "omnivision", but no dts
> files and examples don't count.
>
> The stock ticker is ovti, so please use that.

That's what I sent:
http://patchwork.ozlabs.org/patch/416685/

Could you apply it?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lad, Prabhakar Feb. 4, 2015, 3:27 p.m. UTC | #5
On Wed, Feb 4, 2015 at 3:23 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Rob,
>
> On Wed, Feb 4, 2015 at 12:55 PM, Rob Herring <robherring2@gmail.com> wrote:
>
>> I'm surprised there are not already compatible strings with
>> OmniVision. There are some examples using "omnivision", but no dts
>> files and examples don't count.
>>
>> The stock ticker is ovti, so please use that.
>
> That's what I sent:
> http://patchwork.ozlabs.org/patch/416685/
>
Great I'll rebase the v2 on it.

Cheers,
--Prabhakar Lad
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Feb. 4, 2015, 4:52 p.m. UTC | #6
On Wed, Feb 4, 2015 at 9:23 AM, Fabio Estevam <festevam@gmail.com> wrote:
> Rob,
>
> On Wed, Feb 4, 2015 at 12:55 PM, Rob Herring <robherring2@gmail.com> wrote:
>
>> I'm surprised there are not already compatible strings with
>> OmniVision. There are some examples using "omnivision", but no dts
>> files and examples don't count.
>>
>> The stock ticker is ovti, so please use that.
>
> That's what I sent:
> http://patchwork.ozlabs.org/patch/416685/
>
> Could you apply it?

Yes. Now applied.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Feb. 4, 2015, 5:03 p.m. UTC | #7
Hi Prabhakar,

Thank you for the patch. Here's a partial review.

On Thursday 15 January 2015 23:39:23 Lad, Prabhakar wrote:
> From: Benoit Parrot <bparrot@ti.com>
> 
> this patch adds support for omnivision's ov2659
> sensor.
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> ---
>  .../devicetree/bindings/media/i2c/ov2659.txt       |   33 +
>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>  MAINTAINERS                                        |   10 +
>  drivers/media/i2c/Kconfig                          |   11 +
>  drivers/media/i2c/Makefile                         |    1 +
>  drivers/media/i2c/ov2659.c                         | 1623 +++++++++++++++++
>  include/media/ov2659.h                             |   33 +
>  7 files changed, 1712 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2659.txt
>  create mode 100644 drivers/media/i2c/ov2659.c
>  create mode 100644 include/media/ov2659.h
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> b/Documentation/devicetree/bindings/media/i2c/ov2659.txt new file mode
> 100644
> index 0000000..fc49f44
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> @@ -0,0 +1,33 @@
> +* OV2659 1/5-Inch 2Mp SOC Camera
> +
> +The OV2659 is a 1/5-inch SOC camera, with an active array size of 1632H x
> 1212V.
> +It is programmable through a SCCB.
> +
> +Required Properties:
> +- compatible: Must be "ovt,ov2659"
> +
> +- reg: I2C slave address
> +
> +- clock-frequency: Input clock frequency.
> +
> +For further reading on port node refer to
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +
> +	i2c0@1c22000 {
> +		...
> +		...
> +		 ov2659@30 {
> +			compatible = "ovt,ov2659";
> +			reg = <0x30>;
> +
> +			port {
> +				ov2659_0: endpoint {
> +					clock-frequency = <12000000>;
> +					remote-endpoint = <&vpfe_ep>;
> +				};
> +			};
> +		};
> +		...
> +	};

[snip]

> diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
> new file mode 100644
> index 0000000..ce8ec8d
> --- /dev/null
> +++ b/drivers/media/i2c/ov2659.c
> @@ -0,0 +1,1623 @@

[snip]

> +static int debug;
> +module_param(debug, int, 0644);
> +MODULE_PARM_DESC(debug, "Debug level (0-2)");

The debug parameter is printed in the probe function and then unused. You can 
remove it.

[snip]

> +struct ov2659 {
> +	struct v4l2_subdev sd;
> +	struct media_pad pad;
> +	enum v4l2_mbus_type bus_type;
> +	const struct ov2659_platform_data *pdata;
> +
> +	/* Protects the struct fields below */
> +	struct mutex lock;
> +
> +	struct i2c_client *client;
> +
> +	unsigned short id;

The id field is only used at probe time, you can make it a local variable.

> +	const struct ov2659_framesize *frame_size;
> +	/* Current Output format Register Value (REG_FORMAT_CTRL00) */
> +	struct sensor_register *format_ctrl_regs;
> +
> +	struct v4l2_mbus_framefmt format;
> +
> +	/* Sensor specific feq/pll config */
> +	struct ov2659_pll_ctrl pll;
> +
> +	int streaming;
> +	int power;
> +};

[snip]

> +static const struct ov2659_framesize ov2659_framesizes[] = {
> +	{ /* QVGA */
> +		.width		= 320,
> +		.height		= 240,
> +		.regs		= ov2659_qvga,
> +		.max_exp_lines	= 248,
> +	}, { /* VGA */
> +		.width		= 640,
> +		.height		= 480,
> +		.regs		= ov2659_vga,
> +		.max_exp_lines	= 498,
> +	}, { /* SVGA */
> +		.width		= 800,
> +		.height		= 600,
> +		.regs		= ov2659_svga,
> +		.max_exp_lines	= 498,
> +	}, { /* XGA */
> +		.width		= 1024,
> +		.height		= 768,
> +		.regs		= ov2659_xga,
> +		.max_exp_lines	= 498,
> +	}, { /* 720P */
> +		.width		= 1280,
> +		.height		= 720,
> +		.regs		= ov2659_720p,
> +		.max_exp_lines	= 498,
> +	}, { /* SXGA */
> +		.width		= 1280,
> +		.height		= 1024,
> +		.regs		= ov2659_sxga,
> +		.max_exp_lines	= 1048,
> +	}, { /* UXGA */
> +		.width		= 1600,
> +		.height		= 1200,
> +		.regs		= ov2659_uxga,
> +		.max_exp_lines	= 498,
> +	},
> +};

That's what bothers me the most about drivers for Omnivision sensors. For some 
reason (I'd bet on lack of proper documentation) they list a couple of 
supported resolutions with corresponding register values, instead of computing 
the register values from the format configured by userspace. That's not the 
way we want to go. Prabhakar, do you have enough documentation to fix that ?

[snip]

> +/* sensor register write */
> +static int ov2659_write(struct i2c_client *client, u16 reg, u8 val)
> +{
> +	struct i2c_msg msg[1];
> +	int ret, i;
> +	u8 buf[3];
> +
> +	buf[0] = reg >> 8;
> +	buf[1] = reg & 0xFF;
> +	buf[2] = val;
> +
> +	msg->addr = client->addr;
> +	msg->flags = client->flags;
> +	msg->buf = buf;
> +	msg->len = sizeof(buf);
> +
> +	for (i = 0; i < 5; i++) {

Is the loop really needed, or just copied from a different driver ?

> +		ret = i2c_transfer(client->adapter, msg, 1);
> +		if (ret >= 0)
> +			return 0;
> +
> +		dev_dbg(&client->dev,
> +			"ov2659 write reg(0x%x val:0x%x) failed !\n", reg, val);
> +		udelay(10);
> +	}
> +
> +	return ret;
> +}
> +
> +/* sensor register read */
> +static int ov2659_read(struct i2c_client *client, u16 reg, u8 *val)
> +{
> +	struct i2c_msg msg[2];
> +	int ret, i;
> +	u8 buf[2];
> +
> +	buf[0] = reg >> 8;
> +	buf[1] = reg & 0xFF;
> +
> +	msg[0].addr = client->addr;
> +	msg[0].flags = client->flags;
> +	msg[0].buf = buf;
> +	msg[0].len = sizeof(buf);
> +
> +	msg[1].addr = client->addr;
> +	msg[1].flags = client->flags|I2C_M_RD;

checkpatch.pl should have warned about this. Could you please fix checkpatch 
errors and warnings ?

> +	msg[1].buf = buf;
> +	msg[1].len = 1;
> +
> +	for (i = 0; i < 5; i++) {
> +		ret = i2c_transfer(client->adapter, msg, 2);
> +		if (ret >= 0) {
> +			*val = buf[0];
> +			return 0;
> +		}
> +
> +		dev_dbg(&client->dev,
> +			"ov2659 read reg(0x%x val:0x%x) failed !\n", reg, *val);
> +		udelay(10);
> +	}
> +
> +	return ret;
> +}
> +
> +static int ov2659_write_array(struct i2c_client *client,
> +			      const struct sensor_register *regs)
> +{
> +	int i, ret = 0;
> +
> +	for (i = 0; ret == 0 && regs[i].addr; i++) {
> +		ret = ov2659_write(client, regs[i].addr, regs[i].value);
> +		usleep_range(5000, 6000);

That will result in a large delay when the array grows. Is it really needed ? 
Waiting might be needed after writing some of the registers, but most probably 
not most of them.

> +	}
> +
> +	return ret;
> +}
> +
> +static int dump_reg(struct i2c_client *client, u16 reg)
> +{
> +	u8 val = 0;
> +	int ret;
> +
> +	ret = ov2659_read(client, reg, &val);
> +	dev_dbg(&client->dev, "%s: 0x%04x: 0x%02x\n",
> +				__func__, reg, val);
> +	return ret;
> +}
> +
> +static void ov2659_reg_dump(struct i2c_client *client)
> +{
> +	int i;
> +
> +	dump_reg(client, REG_SOFTWARE_STANDBY);
> +	dump_reg(client, REG_SOFTWARE_RESET);
> +
> +	for (i = 0x3000; i <= 0x302f; i++)
> +		dump_reg(client, i);
> +
> +	for (i = 0x3400; i <= 0x3406; i++)
> +		dump_reg(client, i);
> +
> +	for (i = 0x3500; i <= 0x3513; i++)
> +		dump_reg(client, i);
> +
> +	for (i = 0x3600; i <= 0x3640; i++)
> +		dump_reg(client, i);
> +
> +	for (i = 0x3800; i <= 0x3821; i++)
> +		dump_reg(client, i);
> +
> +	for (i = 0x3a00; i <= 0x3a26; i++)
> +		dump_reg(client, i);
> +
> +	for (i = 0x4000; i <= 0x4009; i++)
> +		dump_reg(client, i);
> +
> +	for (i = 0x4201; i <= 0x4202; i++)
> +		dump_reg(client, i);
> +
> +	for (i = 0x4300; i <= 0x4301; i++)
> +		dump_reg(client, i);
> +
> +	for (i = 0x4600; i <= 0x4609; i++)
> +		dump_reg(client, i);
> +
> +	for (i = 0x4700; i <= 0x4709; i++)
> +		dump_reg(client, i);
> +
> +	for (i = 0x5000; i <= 0x50a0; i++)
> +		dump_reg(client, i);
> +
> +	for (i = 0x5600; i <= 0x5606; i++)
> +		dump_reg(client, i);

That's pretty rough. I'm not sure such a detailed level of debugging is 
needed. You should at least return at the beginning of the function if DEBUG 
isn't defined, to avoid reading all registers without printing their value.

> +}

[snip]

> +static void __ov2659_set_power(struct ov2659 *ov2659, int on)
> +{
> +	struct i2c_client *client = ov2659->client;
> +
> +	on = !!on;
> +
> +	dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
> +
> +	if (ov2659->power == on)
> +		return;
> +
> +	ov2659->power = on;

This seems to be a no-op. I would either remove this function, or, better, add 
regulators support to the driver.

> +}
> +
> +static int ov2659_s_power(struct v4l2_subdev *sd, int on)
> +{
> +	struct ov2659 *ov2659 = to_ov2659(sd);
> +	struct i2c_client *client = ov2659->client;
> +
> +	dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
> +
> +	mutex_lock(&ov2659->lock);
> +	__ov2659_set_power(ov2659, on);
> +	mutex_unlock(&ov2659->lock);
> +
> +	return 0;
> +}

[snip]

> +static int ov2659_s_stream(struct v4l2_subdev *sd, int on)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct ov2659 *ov2659 = to_ov2659(sd);
> +	int ret;
> +
> +	on = !!on;
> +
> +	dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
> +
> +	if (ov2659->streaming == on)
> +		return 0;
> +
> +	mutex_lock(&ov2659->lock);

Shouldn't you protect the above check as well ?

> +
> +	if (!on) {
> +		/* Stop Streaming Sequence */
> +		ov2659_set_streaming(ov2659, 0);
> +		ov2659->streaming = on;
> +		__ov2659_set_power(ov2659, 0);
> +		mutex_unlock(&ov2659->lock);
> +
> +		return 0;
> +	}
> +
> +	/* Start Streaming Sequence */
> +	__ov2659_set_power(ov2659, 1);
> +
> +	ret = ov2659_write_array(client, ov2659_init_regs);
> +	if (ret)
> +		return ret;
> +
> +	ov2659_set_pixel_clock(ov2659);
> +	ov2659_set_frame_size(ov2659);
> +	ov2659_set_format(ov2659);
> +
> +	ov2659_set_streaming(ov2659, 1);
> +
> +	ov2659_reg_dump(client);
> +	ov2659->streaming = on;
> +	mutex_unlock(&ov2659->lock);
> +
> +	return 0;
> +}

[snip]

> +static int ov2659_detect_sensor(struct v4l2_subdev *sd)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct ov2659 *ov2659 = to_ov2659(sd);
> +	u8 pid, ver;
> +	int ret;
> +
> +	dev_dbg(&client->dev, "%s:\n", __func__);
> +
> +	mutex_lock(&ov2659->lock);

As sensor detection is only performed from the probe function before 
registering the subdev I don't think locking is needed here.

> +	 __ov2659_set_power(ov2659, 1);
> +	usleep_range(25000, 26000);
> +
> +	/* soft reset */
> +	ret = ov2659_write(client, REG_SOFTWARE_RESET, 0x01);
> +	if (ret != 0) {
> +		dev_err(&client->dev, "Sensor soft reset failed\n");
> +		ret = -ENODEV;

ret is then ignored.

> +	}
> +	mdelay(5);

Don't delay, please sleep.

> +
> +	/* Check sensor revision */
> +	ret = ov2659_read(client, REG_SC_CHIP_ID_H, &pid);
> +	if (!ret)
> +		ret = ov2659_read(client, REG_SC_CHIP_ID_L, &ver);
> +
> +	__ov2659_set_power(ov2659, 0);
> +
> +	if (!ret) {
> +		ov2659->id = OV265X_ID(pid, ver);
> +		if (ov2659->id != OV2659_ID) {
> +			dev_err(&client->dev,
> +				"Sensor detection failed (%04X, %d)\n",
> +				ov2659->id, ret);
> +			ret = -ENODEV;
> +		} else {
> +			dev_info(&client->dev, "Found OV%04X sensor\n",
> +				ov2659->id);
> +		}
> +	}
> +	mutex_unlock(&ov2659->lock);
> +
> +	return ret;
> +}
> +
> +static struct ov2659_platform_data *
> +ov2659_get_pdata(struct i2c_client *client)
> +{
> +	struct ov2659_platform_data *pdata;
> +	struct device_node *endpoint;
> +	int ret;
> +
> +	dev_dbg(&client->dev, "ov2659_get_pdata invoked\n");
> +
> +	if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
> +		return client->dev.platform_data;
> +
> +	dev_dbg(&client->dev, "ov2659_get_pdata: DT Node found\n");
> +
> +	endpoint = of_graph_get_next_endpoint(client->dev.of_node, NULL);
> +	if (!endpoint)
> +		return NULL;
> +
> +	dev_dbg(&client->dev, "ov2659_get_pdata: endpoint found\n");
> +
> +	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		goto done;
> +
> +	ret = of_property_read_u32(endpoint, "clock-frequency",
> +				   &pdata->mclk_frequency);
> +	if (ret < 0) {
> +		pdata->mclk_frequency = OV2659_DEFAULT_MCLK_FREQ;
> +		dev_info(&client->dev, "using default %u Hz clock frequency\n",
> +			 pdata->mclk_frequency);
> +	}

The clock-frequency property is defined as mandatory, a default case is thus 
not needed.

I would add CCF support to the driver and request the input clock (which is 
called xvclk and not mclk) at probe time. You could then set the clock 
frequency to the value specified in DT, and read it back to get the exact 
frequency provided by the clock source.

> +done:
> +	of_node_put(endpoint);
> +	return pdata;
> +}
> +
> +static int ov2659_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	const struct ov2659_platform_data *pdata = ov2659_get_pdata(client);
> +	struct v4l2_subdev *sd;
> +	struct ov2659 *ov2659;
> +	int ret;
> +
> +	dev_info(&client->dev, "ov2659_probe: debug: %d\n", debug);
> +
> +	if (!pdata) {
> +		dev_err(&client->dev, "platform data not specified\n");
> +		return -EINVAL;
> +	}
> +
> +	ov2659 = devm_kzalloc(&client->dev, sizeof(*ov2659), GFP_KERNEL);
> +	if (!ov2659)
> +		return -ENOMEM;
> +
> +	ov2659->pdata = pdata;
> +	mutex_init(&ov2659->lock);
> +	ov2659->client = client;
> +
> +	sd = &ov2659->sd;
> +	client->flags |= I2C_CLIENT_SCCB;
> +	v4l2_i2c_subdev_init(sd, client, &ov2659_subdev_ops);
> +	strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
> +
> +	sd->internal_ops = &ov2659_subdev_internal_ops;
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +		     V4L2_SUBDEV_FL_HAS_EVENTS;
> +
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	ov2659->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
> +	ret = media_entity_init(&sd->entity, 1, &ov2659->pad, 0);
> +	if (ret < 0)
> +		return ret;
> +#endif
> +
> +	ov2659_get_default_format(&ov2659->format);
> +	ov2659->frame_size = &ov2659_framesizes[2];
> +	ov2659->format_ctrl_regs = ov2659_formats[0].format_ctrl_regs;
> +
> +	ret = ov2659_detect_sensor(sd);
> +	if (ret < 0)
> +		goto error;
> +
> +	/* Calculate the PLL register value needed */
> +	ov2659_pll_calc_params(ov2659);
> +
> +	ret = v4l2_async_register_subdev(&ov2659->sd);
> +	if (ret)
> +		goto error;
> +
> +	dev_info(&client->dev, "%s sensor driver registered !!\n", sd->name);
> +
> +	return 0;
> +
> +error:
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	media_entity_cleanup(&sd->entity);
> +#endif
> +	return ret;
> +}
> +
> +static int ov2659_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +
> +	v4l2_device_unregister_subdev(sd);
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	media_entity_cleanup(&sd->entity);
> +#endif
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ov2659_id[] = {
> +	{ "ov2659", 0 },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(i2c, ov2659_id);
> +
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id ov2659_of_match[] = {
> +	{ .compatible = "ovt,ov2659", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ov2659_of_match);
> +#endif
> +
> +static struct i2c_driver ov2659_i2c_driver = {
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.name	= DRIVER_NAME,
> +		.of_match_table = of_match_ptr(ov2659_of_match),
> +	},
> +	.probe		= ov2659_probe,
> +	.remove		= ov2659_remove,
> +	.id_table	= ov2659_id,
> +};
> +
> +module_i2c_driver(ov2659_i2c_driver);
> +
> +MODULE_AUTHOR("Benoit Parrot <bparrot@ti.com>");
> +MODULE_DESCRIPTION("OV2659 CMOS Image Sensor driver");
> +MODULE_LICENSE("GPL");
Lad, Prabhakar Feb. 4, 2015, 8:55 p.m. UTC | #8
Hi Laurent,

Thanks for the review.

On Wed, Feb 4, 2015 at 5:03 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Prabhakar,
>
> Thank you for the patch. Here's a partial review.
>
> On Thursday 15 January 2015 23:39:23 Lad, Prabhakar wrote:
>> From: Benoit Parrot <bparrot@ti.com>
>>
>> this patch adds support for omnivision's ov2659
>> sensor.
>>
>> Signed-off-by: Benoit Parrot <bparrot@ti.com>
>> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>> ---
>>  .../devicetree/bindings/media/i2c/ov2659.txt       |   33 +
>>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>>  MAINTAINERS                                        |   10 +
>>  drivers/media/i2c/Kconfig                          |   11 +
>>  drivers/media/i2c/Makefile                         |    1 +
>>  drivers/media/i2c/ov2659.c                         | 1623 +++++++++++++++++
>>  include/media/ov2659.h                             |   33 +
>>  7 files changed, 1712 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2659.txt
>>  create mode 100644 drivers/media/i2c/ov2659.c
>>  create mode 100644 include/media/ov2659.h
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov2659.txt
>> b/Documentation/devicetree/bindings/media/i2c/ov2659.txt new file mode
>> 100644
>> index 0000000..fc49f44
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
>> @@ -0,0 +1,33 @@
>> +* OV2659 1/5-Inch 2Mp SOC Camera
>> +
>> +The OV2659 is a 1/5-inch SOC camera, with an active array size of 1632H x
>> 1212V.
>> +It is programmable through a SCCB.
>> +
>> +Required Properties:
>> +- compatible: Must be "ovt,ov2659"
>> +
>> +- reg: I2C slave address
>> +
>> +- clock-frequency: Input clock frequency.
>> +
>> +For further reading on port node refer to
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example:
>> +
>> +     i2c0@1c22000 {
>> +             ...
>> +             ...
>> +              ov2659@30 {
>> +                     compatible = "ovt,ov2659";
>> +                     reg = <0x30>;
>> +
>> +                     port {
>> +                             ov2659_0: endpoint {
>> +                                     clock-frequency = <12000000>;
>> +                                     remote-endpoint = <&vpfe_ep>;
>> +                             };
>> +                     };
>> +             };
>> +             ...
>> +     };
>
> [snip]
>
>> diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
>> new file mode 100644
>> index 0000000..ce8ec8d
>> --- /dev/null
>> +++ b/drivers/media/i2c/ov2659.c
>> @@ -0,0 +1,1623 @@
>
> [snip]
>
>> +static int debug;
>> +module_param(debug, int, 0644);
>> +MODULE_PARM_DESC(debug, "Debug level (0-2)");
>
> The debug parameter is printed in the probe function and then unused. You can
> remove it.
>
OK will drop it.

> [snip]
>
>> +struct ov2659 {
>> +     struct v4l2_subdev sd;
>> +     struct media_pad pad;
>> +     enum v4l2_mbus_type bus_type;
>> +     const struct ov2659_platform_data *pdata;
>> +
>> +     /* Protects the struct fields below */
>> +     struct mutex lock;
>> +
>> +     struct i2c_client *client;
>> +
>> +     unsigned short id;
>
> The id field is only used at probe time, you can make it a local variable.
>
OK will make local.

>> +     const struct ov2659_framesize *frame_size;
>> +     /* Current Output format Register Value (REG_FORMAT_CTRL00) */
>> +     struct sensor_register *format_ctrl_regs;
>> +
>> +     struct v4l2_mbus_framefmt format;
>> +
>> +     /* Sensor specific feq/pll config */
>> +     struct ov2659_pll_ctrl pll;
>> +
>> +     int streaming;
>> +     int power;
>> +};
>
> [snip]
>
>> +static const struct ov2659_framesize ov2659_framesizes[] = {
>> +     { /* QVGA */
>> +             .width          = 320,
>> +             .height         = 240,
>> +             .regs           = ov2659_qvga,
>> +             .max_exp_lines  = 248,
>> +     }, { /* VGA */
>> +             .width          = 640,
>> +             .height         = 480,
>> +             .regs           = ov2659_vga,
>> +             .max_exp_lines  = 498,
>> +     }, { /* SVGA */
>> +             .width          = 800,
>> +             .height         = 600,
>> +             .regs           = ov2659_svga,
>> +             .max_exp_lines  = 498,
>> +     }, { /* XGA */
>> +             .width          = 1024,
>> +             .height         = 768,
>> +             .regs           = ov2659_xga,
>> +             .max_exp_lines  = 498,
>> +     }, { /* 720P */
>> +             .width          = 1280,
>> +             .height         = 720,
>> +             .regs           = ov2659_720p,
>> +             .max_exp_lines  = 498,
>> +     }, { /* SXGA */
>> +             .width          = 1280,
>> +             .height         = 1024,
>> +             .regs           = ov2659_sxga,
>> +             .max_exp_lines  = 1048,
>> +     }, { /* UXGA */
>> +             .width          = 1600,
>> +             .height         = 1200,
>> +             .regs           = ov2659_uxga,
>> +             .max_exp_lines  = 498,
>> +     },
>> +};
>
> That's what bothers me the most about drivers for Omnivision sensors. For some
> reason (I'd bet on lack of proper documentation) they list a couple of
> supported resolutions with corresponding register values, instead of computing
> the register values from the format configured by userspace. That's not the
> way we want to go. Prabhakar, do you have enough documentation to fix that ?
>
I am afraid I have limited documentation here.

> [snip]
>
>> +/* sensor register write */
>> +static int ov2659_write(struct i2c_client *client, u16 reg, u8 val)
>> +{
>> +     struct i2c_msg msg[1];
>> +     int ret, i;
>> +     u8 buf[3];
>> +
>> +     buf[0] = reg >> 8;
>> +     buf[1] = reg & 0xFF;
>> +     buf[2] = val;
>> +
>> +     msg->addr = client->addr;
>> +     msg->flags = client->flags;
>> +     msg->buf = buf;
>> +     msg->len = sizeof(buf);
>> +
>> +     for (i = 0; i < 5; i++) {
>
> Is the loop really needed, or just copied from a different driver ?
>
not really, just incase if transfer fails retrys.

>> +             ret = i2c_transfer(client->adapter, msg, 1);
>> +             if (ret >= 0)
>> +                     return 0;
>> +
>> +             dev_dbg(&client->dev,
>> +                     "ov2659 write reg(0x%x val:0x%x) failed !\n", reg, val);
>> +             udelay(10);
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +/* sensor register read */
>> +static int ov2659_read(struct i2c_client *client, u16 reg, u8 *val)
>> +{
>> +     struct i2c_msg msg[2];
>> +     int ret, i;
>> +     u8 buf[2];
>> +
>> +     buf[0] = reg >> 8;
>> +     buf[1] = reg & 0xFF;
>> +
>> +     msg[0].addr = client->addr;
>> +     msg[0].flags = client->flags;
>> +     msg[0].buf = buf;
>> +     msg[0].len = sizeof(buf);
>> +
>> +     msg[1].addr = client->addr;
>> +     msg[1].flags = client->flags|I2C_M_RD;
>
> checkpatch.pl should have warned about this. Could you please fix checkpatch
> errors and warnings ?
>
strange this was checkpatched, anyway will fix it.

>> +     msg[1].buf = buf;
>> +     msg[1].len = 1;
>> +
>> +     for (i = 0; i < 5; i++) {
>> +             ret = i2c_transfer(client->adapter, msg, 2);
>> +             if (ret >= 0) {
>> +                     *val = buf[0];
>> +                     return 0;
>> +             }
>> +
>> +             dev_dbg(&client->dev,
>> +                     "ov2659 read reg(0x%x val:0x%x) failed !\n", reg, *val);
>> +             udelay(10);
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +static int ov2659_write_array(struct i2c_client *client,
>> +                           const struct sensor_register *regs)
>> +{
>> +     int i, ret = 0;
>> +
>> +     for (i = 0; ret == 0 && regs[i].addr; i++) {
>> +             ret = ov2659_write(client, regs[i].addr, regs[i].value);
>> +             usleep_range(5000, 6000);
>
> That will result in a large delay when the array grows. Is it really needed ?
> Waiting might be needed after writing some of the registers, but most probably
> not most of them.
>
Ok will verify it.


>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +static int dump_reg(struct i2c_client *client, u16 reg)
>> +{
>> +     u8 val = 0;
>> +     int ret;
>> +
>> +     ret = ov2659_read(client, reg, &val);
>> +     dev_dbg(&client->dev, "%s: 0x%04x: 0x%02x\n",
>> +                             __func__, reg, val);
>> +     return ret;
>> +}
>> +
>> +static void ov2659_reg_dump(struct i2c_client *client)
>> +{
>> +     int i;
>> +
>> +     dump_reg(client, REG_SOFTWARE_STANDBY);
>> +     dump_reg(client, REG_SOFTWARE_RESET);
>> +
>> +     for (i = 0x3000; i <= 0x302f; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x3400; i <= 0x3406; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x3500; i <= 0x3513; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x3600; i <= 0x3640; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x3800; i <= 0x3821; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x3a00; i <= 0x3a26; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x4000; i <= 0x4009; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x4201; i <= 0x4202; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x4300; i <= 0x4301; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x4600; i <= 0x4609; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x4700; i <= 0x4709; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x5000; i <= 0x50a0; i++)
>> +             dump_reg(client, i);
>> +
>> +     for (i = 0x5600; i <= 0x5606; i++)
>> +             dump_reg(client, i);
>
> That's pretty rough. I'm not sure such a detailed level of debugging is
> needed. You should at least return at the beginning of the function if DEBUG
> isn't defined, to avoid reading all registers without printing their value.
>
OK will fix it.

>> +}
>
> [snip]
>
>> +static void __ov2659_set_power(struct ov2659 *ov2659, int on)
>> +{
>> +     struct i2c_client *client = ov2659->client;
>> +
>> +     on = !!on;
>> +
>> +     dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
>> +
>> +     if (ov2659->power == on)
>> +             return;
>> +
>> +     ov2659->power = on;
>
> This seems to be a no-op. I would either remove this function, or, better, add
> regulators support to the driver.
>
Will drop it.

>> +}
>> +
>> +static int ov2659_s_power(struct v4l2_subdev *sd, int on)
>> +{
>> +     struct ov2659 *ov2659 = to_ov2659(sd);
>> +     struct i2c_client *client = ov2659->client;
>> +
>> +     dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
>> +
>> +     mutex_lock(&ov2659->lock);
>> +     __ov2659_set_power(ov2659, on);
>> +     mutex_unlock(&ov2659->lock);
>> +
>> +     return 0;
>> +}
>
> [snip]
>
>> +static int ov2659_s_stream(struct v4l2_subdev *sd, int on)
>> +{
>> +     struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +     struct ov2659 *ov2659 = to_ov2659(sd);
>> +     int ret;
>> +
>> +     on = !!on;
>> +
>> +     dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
>> +
>> +     if (ov2659->streaming == on)
>> +             return 0;
>> +
>> +     mutex_lock(&ov2659->lock);
>
> Shouldn't you protect the above check as well ?
>
Yes, will fix it.

>> +
>> +     if (!on) {
>> +             /* Stop Streaming Sequence */
>> +             ov2659_set_streaming(ov2659, 0);
>> +             ov2659->streaming = on;
>> +             __ov2659_set_power(ov2659, 0);
>> +             mutex_unlock(&ov2659->lock);
>> +
>> +             return 0;
>> +     }
>> +
>> +     /* Start Streaming Sequence */
>> +     __ov2659_set_power(ov2659, 1);
>> +
>> +     ret = ov2659_write_array(client, ov2659_init_regs);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ov2659_set_pixel_clock(ov2659);
>> +     ov2659_set_frame_size(ov2659);
>> +     ov2659_set_format(ov2659);
>> +
>> +     ov2659_set_streaming(ov2659, 1);
>> +
>> +     ov2659_reg_dump(client);
>> +     ov2659->streaming = on;
>> +     mutex_unlock(&ov2659->lock);
>> +
>> +     return 0;
>> +}
>
> [snip]
>
>> +static int ov2659_detect_sensor(struct v4l2_subdev *sd)
>> +{
>> +     struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +     struct ov2659 *ov2659 = to_ov2659(sd);
>> +     u8 pid, ver;
>> +     int ret;
>> +
>> +     dev_dbg(&client->dev, "%s:\n", __func__);
>> +
>> +     mutex_lock(&ov2659->lock);
>
> As sensor detection is only performed from the probe function before
> registering the subdev I don't think locking is needed here.
>
OK will drop it.

>> +      __ov2659_set_power(ov2659, 1);
>> +     usleep_range(25000, 26000);
>> +
>> +     /* soft reset */
>> +     ret = ov2659_write(client, REG_SOFTWARE_RESET, 0x01);
>> +     if (ret != 0) {
>> +             dev_err(&client->dev, "Sensor soft reset failed\n");
>> +             ret = -ENODEV;
>
> ret is then ignored.
>
will return ret.

>> +     }
>> +     mdelay(5);
>
> Don't delay, please sleep.
>
OK

>> +
>> +     /* Check sensor revision */
>> +     ret = ov2659_read(client, REG_SC_CHIP_ID_H, &pid);
>> +     if (!ret)
>> +             ret = ov2659_read(client, REG_SC_CHIP_ID_L, &ver);
>> +
>> +     __ov2659_set_power(ov2659, 0);
>> +
>> +     if (!ret) {
>> +             ov2659->id = OV265X_ID(pid, ver);
>> +             if (ov2659->id != OV2659_ID) {
>> +                     dev_err(&client->dev,
>> +                             "Sensor detection failed (%04X, %d)\n",
>> +                             ov2659->id, ret);
>> +                     ret = -ENODEV;
>> +             } else {
>> +                     dev_info(&client->dev, "Found OV%04X sensor\n",
>> +                             ov2659->id);
>> +             }
>> +     }
>> +     mutex_unlock(&ov2659->lock);
>> +
>> +     return ret;
>> +}
>> +
>> +static struct ov2659_platform_data *
>> +ov2659_get_pdata(struct i2c_client *client)
>> +{
>> +     struct ov2659_platform_data *pdata;
>> +     struct device_node *endpoint;
>> +     int ret;
>> +
>> +     dev_dbg(&client->dev, "ov2659_get_pdata invoked\n");
>> +
>> +     if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
>> +             return client->dev.platform_data;
>> +
>> +     dev_dbg(&client->dev, "ov2659_get_pdata: DT Node found\n");
>> +
>> +     endpoint = of_graph_get_next_endpoint(client->dev.of_node, NULL);
>> +     if (!endpoint)
>> +             return NULL;
>> +
>> +     dev_dbg(&client->dev, "ov2659_get_pdata: endpoint found\n");
>> +
>> +     pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>> +     if (!pdata)
>> +             goto done;
>> +
>> +     ret = of_property_read_u32(endpoint, "clock-frequency",
>> +                                &pdata->mclk_frequency);
>> +     if (ret < 0) {
>> +             pdata->mclk_frequency = OV2659_DEFAULT_MCLK_FREQ;
>> +             dev_info(&client->dev, "using default %u Hz clock frequency\n",
>> +                      pdata->mclk_frequency);
>> +     }
>
> The clock-frequency property is defined as mandatory, a default case is thus
> not needed.
>
OK will drop it.

> I would add CCF support to the driver and request the input clock (which is
> called xvclk and not mclk) at probe time. You could then set the clock
> frequency to the value specified in DT, and read it back to get the exact
> frequency provided by the clock source.
>
Ok will add CCF.

Regards,
--Prabhakar Lad
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Feb. 5, 2015, 11:53 a.m. UTC | #9
Hi Prabhakar,

On Wednesday 04 February 2015 20:55:02 Lad, Prabhakar wrote:
> On Wed, Feb 4, 2015 at 5:03 PM, Laurent Pinchart wrote:
> > On Thursday 15 January 2015 23:39:23 Lad, Prabhakar wrote:
> >> From: Benoit Parrot <bparrot@ti.com>
> >> 
> >> this patch adds support for omnivision's ov2659
> >> sensor.
> >> 
> >> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> >> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> >> ---
> >> 
> >>  .../devicetree/bindings/media/i2c/ov2659.txt       |   33 +
> >>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
> >>  MAINTAINERS                                        |   10 +
> >>  drivers/media/i2c/Kconfig                          |   11 +
> >>  drivers/media/i2c/Makefile                         |    1 +
> >>  drivers/media/i2c/ov2659.c                         | 1623
> >>  +++++++++++++++++
> >>  include/media/ov2659.h                             |   33 +
> >>  7 files changed, 1712 insertions(+)
> >>  create mode 100644
> >>  Documentation/devicetree/bindings/media/i2c/ov2659.txt
> >>  create mode 100644 drivers/media/i2c/ov2659.c
> >>  create mode 100644 include/media/ov2659.h

[snip]

> >> diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
> >> new file mode 100644
> >> index 0000000..ce8ec8d
> >> --- /dev/null
> >> +++ b/drivers/media/i2c/ov2659.c
> >> @@ -0,0 +1,1623 @@

[snip]

> >> +static const struct ov2659_framesize ov2659_framesizes[] = {
> >> +     { /* QVGA */
> >> +             .width          = 320,
> >> +             .height         = 240,
> >> +             .regs           = ov2659_qvga,
> >> +             .max_exp_lines  = 248,
> >> +     }, { /* VGA */
> >> +             .width          = 640,
> >> +             .height         = 480,
> >> +             .regs           = ov2659_vga,
> >> +             .max_exp_lines  = 498,
> >> +     }, { /* SVGA */
> >> +             .width          = 800,
> >> +             .height         = 600,
> >> +             .regs           = ov2659_svga,
> >> +             .max_exp_lines  = 498,
> >> +     }, { /* XGA */
> >> +             .width          = 1024,
> >> +             .height         = 768,
> >> +             .regs           = ov2659_xga,
> >> +             .max_exp_lines  = 498,
> >> +     }, { /* 720P */
> >> +             .width          = 1280,
> >> +             .height         = 720,
> >> +             .regs           = ov2659_720p,
> >> +             .max_exp_lines  = 498,
> >> +     }, { /* SXGA */
> >> +             .width          = 1280,
> >> +             .height         = 1024,
> >> +             .regs           = ov2659_sxga,
> >> +             .max_exp_lines  = 1048,
> >> +     }, { /* UXGA */
> >> +             .width          = 1600,
> >> +             .height         = 1200,
> >> +             .regs           = ov2659_uxga,
> >> +             .max_exp_lines  = 498,
> >> +     },
> >> +};
> > 
> > That's what bothers me the most about drivers for Omnivision sensors. For
> > some reason (I'd bet on lack of proper documentation) they list a couple
> > of supported resolutions with corresponding register values, instead of
> > computing the register values from the format configured by userspace.
> > That's not the way we want to go. Prabhakar, do you have enough
> > documentation to fix that ?
>
> I am afraid I have limited documentation here.

How limited ? :-) I assume someone has documentation, given that the patch 
contains a larger number of #define's with register names.
Lad, Prabhakar Feb. 5, 2015, 11:55 a.m. UTC | #10
Hi Laurent,

On Thu, Feb 5, 2015 at 11:53 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Prabhakar,
>
> On Wednesday 04 February 2015 20:55:02 Lad, Prabhakar wrote:
>> On Wed, Feb 4, 2015 at 5:03 PM, Laurent Pinchart wrote:
>> > On Thursday 15 January 2015 23:39:23 Lad, Prabhakar wrote:
>> >> From: Benoit Parrot <bparrot@ti.com>
>> >>
>> >> this patch adds support for omnivision's ov2659
>> >> sensor.
>> >>
>> >> Signed-off-by: Benoit Parrot <bparrot@ti.com>
>> >> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>> >> ---
>> >>
>> >>  .../devicetree/bindings/media/i2c/ov2659.txt       |   33 +
>> >>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>> >>  MAINTAINERS                                        |   10 +
>> >>  drivers/media/i2c/Kconfig                          |   11 +
>> >>  drivers/media/i2c/Makefile                         |    1 +
>> >>  drivers/media/i2c/ov2659.c                         | 1623
>> >>  +++++++++++++++++
>> >>  include/media/ov2659.h                             |   33 +
>> >>  7 files changed, 1712 insertions(+)
>> >>  create mode 100644
>> >>  Documentation/devicetree/bindings/media/i2c/ov2659.txt
>> >>  create mode 100644 drivers/media/i2c/ov2659.c
>> >>  create mode 100644 include/media/ov2659.h
>
> [snip]
>
>> >> diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
>> >> new file mode 100644
>> >> index 0000000..ce8ec8d
>> >> --- /dev/null
>> >> +++ b/drivers/media/i2c/ov2659.c
>> >> @@ -0,0 +1,1623 @@
>
> [snip]
>
>> >> +static const struct ov2659_framesize ov2659_framesizes[] = {
>> >> +     { /* QVGA */
>> >> +             .width          = 320,
>> >> +             .height         = 240,
>> >> +             .regs           = ov2659_qvga,
>> >> +             .max_exp_lines  = 248,
>> >> +     }, { /* VGA */
>> >> +             .width          = 640,
>> >> +             .height         = 480,
>> >> +             .regs           = ov2659_vga,
>> >> +             .max_exp_lines  = 498,
>> >> +     }, { /* SVGA */
>> >> +             .width          = 800,
>> >> +             .height         = 600,
>> >> +             .regs           = ov2659_svga,
>> >> +             .max_exp_lines  = 498,
>> >> +     }, { /* XGA */
>> >> +             .width          = 1024,
>> >> +             .height         = 768,
>> >> +             .regs           = ov2659_xga,
>> >> +             .max_exp_lines  = 498,
>> >> +     }, { /* 720P */
>> >> +             .width          = 1280,
>> >> +             .height         = 720,
>> >> +             .regs           = ov2659_720p,
>> >> +             .max_exp_lines  = 498,
>> >> +     }, { /* SXGA */
>> >> +             .width          = 1280,
>> >> +             .height         = 1024,
>> >> +             .regs           = ov2659_sxga,
>> >> +             .max_exp_lines  = 1048,
>> >> +     }, { /* UXGA */
>> >> +             .width          = 1600,
>> >> +             .height         = 1200,
>> >> +             .regs           = ov2659_uxga,
>> >> +             .max_exp_lines  = 498,
>> >> +     },
>> >> +};
>> >
>> > That's what bothers me the most about drivers for Omnivision sensors. For
>> > some reason (I'd bet on lack of proper documentation) they list a couple
>> > of supported resolutions with corresponding register values, instead of
>> > computing the register values from the format configured by userspace.
>> > That's not the way we want to go. Prabhakar, do you have enough
>> > documentation to fix that ?
>>
>> I am afraid I have limited documentation here.
>
> How limited ? :-) I assume someone has documentation, given that the patch
> contains a larger number of #define's with register names.
>
Yea I don’t have NDA signed with TI/Omnivision :( because I which I
lack the documentation.

Regards,
--Prabhakar
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Feb. 5, 2015, 2:52 p.m. UTC | #11
Hi Prabhakar,

(CC'ing Benoit Parrot)

On Thursday 05 February 2015 11:55:28 Lad, Prabhakar wrote:
> On Thu, Feb 5, 2015 at 11:53 AM, Laurent Pinchart wrote:
> > On Wednesday 04 February 2015 20:55:02 Lad, Prabhakar wrote:
> >> On Wed, Feb 4, 2015 at 5:03 PM, Laurent Pinchart wrote:
> >> > On Thursday 15 January 2015 23:39:23 Lad, Prabhakar wrote:
> >> >> From: Benoit Parrot <bparrot@ti.com>
> >> >> 
> >> >> this patch adds support for omnivision's ov2659
> >> >> sensor.
> >> >> 
> >> >> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> >> >> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> >> >> ---
> >> >> 
> >> >>  .../devicetree/bindings/media/i2c/ov2659.txt       |   33 +
> >> >>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
> >> >>  MAINTAINERS                                        |   10 +
> >> >>  drivers/media/i2c/Kconfig                          |   11 +
> >> >>  drivers/media/i2c/Makefile                         |    1 +
> >> >>  drivers/media/i2c/ov2659.c                         | 1623 +++++++++++
> >> >>  include/media/ov2659.h                             |   33 +
> >> >>  7 files changed, 1712 insertions(+)
> >> >>  create mode 100644
> >> >>  Documentation/devicetree/bindings/media/i2c/ov2659.txt
> >> >>  create mode 100644 drivers/media/i2c/ov2659.c
> >> >>  create mode 100644 include/media/ov2659.h
> > 
> > [snip]
> > 
> >> >> diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
> >> >> new file mode 100644
> >> >> index 0000000..ce8ec8d
> >> >> --- /dev/null
> >> >> +++ b/drivers/media/i2c/ov2659.c
> >> >> @@ -0,0 +1,1623 @@
> > 
> > [snip]
> > 
> >> >> +static const struct ov2659_framesize ov2659_framesizes[] = {
> >> >> +     { /* QVGA */
> >> >> +             .width          = 320,
> >> >> +             .height         = 240,
> >> >> +             .regs           = ov2659_qvga,
> >> >> +             .max_exp_lines  = 248,
> >> >> +     }, { /* VGA */
> >> >> +             .width          = 640,
> >> >> +             .height         = 480,
> >> >> +             .regs           = ov2659_vga,
> >> >> +             .max_exp_lines  = 498,
> >> >> +     }, { /* SVGA */
> >> >> +             .width          = 800,
> >> >> +             .height         = 600,
> >> >> +             .regs           = ov2659_svga,
> >> >> +             .max_exp_lines  = 498,
> >> >> +     }, { /* XGA */
> >> >> +             .width          = 1024,
> >> >> +             .height         = 768,
> >> >> +             .regs           = ov2659_xga,
> >> >> +             .max_exp_lines  = 498,
> >> >> +     }, { /* 720P */
> >> >> +             .width          = 1280,
> >> >> +             .height         = 720,
> >> >> +             .regs           = ov2659_720p,
> >> >> +             .max_exp_lines  = 498,
> >> >> +     }, { /* SXGA */
> >> >> +             .width          = 1280,
> >> >> +             .height         = 1024,
> >> >> +             .regs           = ov2659_sxga,
> >> >> +             .max_exp_lines  = 1048,
> >> >> +     }, { /* UXGA */
> >> >> +             .width          = 1600,
> >> >> +             .height         = 1200,
> >> >> +             .regs           = ov2659_uxga,
> >> >> +             .max_exp_lines  = 498,
> >> >> +     },
> >> >> +};
> >> > 
> >> > That's what bothers me the most about drivers for Omnivision sensors.
> >> > For some reason (I'd bet on lack of proper documentation) they list a
> >> > couple of supported resolutions with corresponding register values,
> >> > instead of computing the register values from the format configured by
> >> > userspace. That's not the way we want to go. Prabhakar, do you have
> >> > enough documentation to fix that ?
> >> 
> >> I am afraid I have limited documentation here.
> > 
> > How limited ? :-) I assume someone has documentation, given that the patch
> > contains a larger number of #define's with register names.
> 
> Yea I don’t have NDA signed with TI/Omnivision :( because I which I
> lack the documentation.

And a quick online search doesn't show any leaked datasheet. Wikileaks isn't 
doing a good job ;-)

Benoit, do you think this is something that can be fixed ?
Benoit Parrot Feb. 10, 2015, 7:36 p.m. UTC | #12
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote on Thu [2015-Feb-05 16:52:22 +0200]:
> Hi Prabhakar,
> 
> (CC'ing Benoit Parrot)
> 
> On Thursday 05 February 2015 11:55:28 Lad, Prabhakar wrote:
> > On Thu, Feb 5, 2015 at 11:53 AM, Laurent Pinchart wrote:
> > > On Wednesday 04 February 2015 20:55:02 Lad, Prabhakar wrote:
> > >> On Wed, Feb 4, 2015 at 5:03 PM, Laurent Pinchart wrote:
> > >> > On Thursday 15 January 2015 23:39:23 Lad, Prabhakar wrote:
> > >> >> From: Benoit Parrot <bparrot@ti.com>
> > >> >> 
> > >> >> this patch adds support for omnivision's ov2659
> > >> >> sensor.
> > >> >> 
> > >> >> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > >> >> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> > >> >> ---
> > >> >> 
> > >> >>  .../devicetree/bindings/media/i2c/ov2659.txt       |   33 +
> > >> >>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
> > >> >>  MAINTAINERS                                        |   10 +
> > >> >>  drivers/media/i2c/Kconfig                          |   11 +
> > >> >>  drivers/media/i2c/Makefile                         |    1 +
> > >> >>  drivers/media/i2c/ov2659.c                         | 1623 +++++++++++
> > >> >>  include/media/ov2659.h                             |   33 +
> > >> >>  7 files changed, 1712 insertions(+)
> > >> >>  create mode 100644
> > >> >>  Documentation/devicetree/bindings/media/i2c/ov2659.txt
> > >> >>  create mode 100644 drivers/media/i2c/ov2659.c
> > >> >>  create mode 100644 include/media/ov2659.h
> > > 
> > > [snip]
> > > 
> > >> >> diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
> > >> >> new file mode 100644
> > >> >> index 0000000..ce8ec8d
> > >> >> --- /dev/null
> > >> >> +++ b/drivers/media/i2c/ov2659.c
> > >> >> @@ -0,0 +1,1623 @@
> > > 
> > > [snip]
> > > 
> > >> >> +static const struct ov2659_framesize ov2659_framesizes[] = {
> > >> >> +     { /* QVGA */
> > >> >> +             .width          = 320,
> > >> >> +             .height         = 240,
> > >> >> +             .regs           = ov2659_qvga,
> > >> >> +             .max_exp_lines  = 248,
> > >> >> +     }, { /* VGA */
> > >> >> +             .width          = 640,
> > >> >> +             .height         = 480,
> > >> >> +             .regs           = ov2659_vga,
> > >> >> +             .max_exp_lines  = 498,
> > >> >> +     }, { /* SVGA */
> > >> >> +             .width          = 800,
> > >> >> +             .height         = 600,
> > >> >> +             .regs           = ov2659_svga,
> > >> >> +             .max_exp_lines  = 498,
> > >> >> +     }, { /* XGA */
> > >> >> +             .width          = 1024,
> > >> >> +             .height         = 768,
> > >> >> +             .regs           = ov2659_xga,
> > >> >> +             .max_exp_lines  = 498,
> > >> >> +     }, { /* 720P */
> > >> >> +             .width          = 1280,
> > >> >> +             .height         = 720,
> > >> >> +             .regs           = ov2659_720p,
> > >> >> +             .max_exp_lines  = 498,
> > >> >> +     }, { /* SXGA */
> > >> >> +             .width          = 1280,
> > >> >> +             .height         = 1024,
> > >> >> +             .regs           = ov2659_sxga,
> > >> >> +             .max_exp_lines  = 1048,
> > >> >> +     }, { /* UXGA */
> > >> >> +             .width          = 1600,
> > >> >> +             .height         = 1200,
> > >> >> +             .regs           = ov2659_uxga,
> > >> >> +             .max_exp_lines  = 498,
> > >> >> +     },
> > >> >> +};
> > >> > 
> > >> > That's what bothers me the most about drivers for Omnivision sensors.
> > >> > For some reason (I'd bet on lack of proper documentation) they list a
> > >> > couple of supported resolutions with corresponding register values,
> > >> > instead of computing the register values from the format configured by
> > >> > userspace. That's not the way we want to go. Prabhakar, do you have
> > >> > enough documentation to fix that ?
> > >> 
> > >> I am afraid I have limited documentation here.
> > > 
> > > How limited ? :-) I assume someone has documentation, given that the patch
> > > contains a larger number of #define's with register names.
> > 
> > Yea I don’t have NDA signed with TI/Omnivision :( because I which I
> > lack the documentation.
> 
> And a quick online search doesn't show any leaked datasheet. Wikileaks isn't 
> doing a good job ;-)
> 
> Benoit, do you think this is something that can be fixed ?

Laurent,

I did spend several days (many moons ago) to try and derive a generic
way to set the output resolution dynamically. However even though
the data sheet is somewhat useful, the various working example config
from the vendor make use of several un-documented registers which
make this pretty much unfeasible.

I am afraid we are stuck with this method for the time being.

Regards,
Benoit Parrot

> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/media/i2c/ov2659.txt b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
new file mode 100644
index 0000000..fc49f44
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
@@ -0,0 +1,33 @@ 
+* OV2659 1/5-Inch 2Mp SOC Camera
+
+The OV2659 is a 1/5-inch SOC camera, with an active array size of 1632H x 1212V.
+It is programmable through a SCCB.
+
+Required Properties:
+- compatible: Must be "ovt,ov2659"
+
+- reg: I2C slave address
+
+- clock-frequency: Input clock frequency.
+
+For further reading on port node refer to
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+
+	i2c0@1c22000 {
+		...
+		...
+		 ov2659@30 {
+			compatible = "ovt,ov2659";
+			reg = <0x30>;
+
+			port {
+				ov2659_0: endpoint {
+					clock-frequency = <12000000>;
+					remote-endpoint = <&vpfe_ep>;
+				};
+			};
+		};
+		...
+	};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index b1df0ad..153cd92 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -118,6 +118,7 @@  nvidia	NVIDIA
 nxp	NXP Semiconductors
 onnn	ON Semiconductor Corp.
 opencores	OpenCores.org
+ovt	OmniVision Technologies
 panasonic	Panasonic Corporation
 pericom	Pericom Technology Inc.
 phytec	PHYTEC Messtechnik GmbH
diff --git a/MAINTAINERS b/MAINTAINERS
index 4318f34..fa62c91 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8754,6 +8754,16 @@  T:	git git://linuxtv.org/mhadli/v4l-dvb-davinci_devices.git
 S:	Maintained
 F:	drivers/media/platform/am437x/
 
+OV2659 OMNIVISION SENSOR DRIVER
+M:	Lad, Prabhakar <prabhakar.csengg@gmail.com>
+L:	linux-media@vger.kernel.org
+W:	http://linuxtv.org/
+Q:	http://patchwork.linuxtv.org/project/linux-media/list/
+T:	git git://linuxtv.org/mhadli/v4l-dvb-davinci_devices.git
+S:	Maintained
+F:	drivers/media/i2c/ov2659.c
+F:	include/media/ov2659.h
+
 SIS 190 ETHERNET DRIVER
 M:	Francois Romieu <romieu@fr.zoreil.com>
 L:	netdev@vger.kernel.org
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index ca84543..47f5042 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -465,6 +465,17 @@  config VIDEO_APTINA_PLL
 config VIDEO_SMIAPP_PLL
 	tristate
 
+config VIDEO_OV2659
+	tristate "OmniVision OV2659 sensor support"
+	depends on VIDEO_V4L2 && I2C
+	depends on MEDIA_CAMERA_SUPPORT
+	---help---
+	  This is a Video4Linux2 sensor-level driver for the OmniVision
+	  OV2659 camera.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ov2659.
+
 config VIDEO_OV7640
 	tristate "OmniVision OV7640 sensor support"
 	depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 98589001..f165fae 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -77,3 +77,4 @@  obj-$(CONFIG_VIDEO_SMIAPP_PLL)	+= smiapp-pll.o
 obj-$(CONFIG_VIDEO_AK881X)		+= ak881x.o
 obj-$(CONFIG_VIDEO_IR_I2C)  += ir-kbd-i2c.o
 obj-$(CONFIG_VIDEO_ML86V7667)	+= ml86v7667.o
+obj-$(CONFIG_VIDEO_OV2659)	+= ov2659.o
diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
new file mode 100644
index 0000000..ce8ec8d
--- /dev/null
+++ b/drivers/media/i2c/ov2659.c
@@ -0,0 +1,1623 @@ 
+/*
+ * Omnivision OV2659 CMOS Image Sensor driver
+ *
+ * Copyright (C) 2015 Texas Instruments, Inc.
+ *
+ * Benoit Parrot <bparrot@ti.com>
+ * Lad, Prabhakar <prabhakar.csengg@gmail.com>
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/media.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/videodev2.h>
+
+#include <media/media-entity.h>
+#include <media/ov2659.h>
+#include <media/v4l2-common.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-image-sizes.h>
+#include <media/v4l2-mediabus.h>
+#include <media/v4l2-of.h>
+#include <media/v4l2-subdev.h>
+
+static int debug;
+module_param(debug, int, 0644);
+MODULE_PARM_DESC(debug, "Debug level (0-2)");
+
+#define DRIVER_NAME "ov2659"
+
+/*
+ * OV2659 register definitions
+ */
+#define REG_SOFTWARE_STANDBY		0x0100
+#define REG_SOFTWARE_RESET		0x0103
+#define REG_IO_CTRL00			0x3000
+#define REG_IO_CTRL01			0x3001
+#define REG_IO_CTRL02			0x3002
+#define REG_OUTPUT_VALUE00		0x3008
+#define REG_OUTPUT_VALUE01		0x3009
+#define REG_OUTPUT_VALUE02		0x300d
+#define REG_OUTPUT_SELECT00		0x300e
+#define REG_OUTPUT_SELECT01		0x300f
+#define REG_OUTPUT_SELECT02		0x3010
+#define REG_OUTPUT_DRIVE		0x3011
+#define REG_INPUT_READOUT00		0x302d
+#define REG_INPUT_READOUT01		0x302e
+#define REG_INPUT_READOUT02		0x302f
+
+#define REG_SC_PLL_CTRL0		0x3003
+#define REG_SC_PLL_CTRL1		0x3004
+#define REG_SC_PLL_CTRL2		0x3005
+#define REG_SC_PLL_CTRL3		0x3006
+#define REG_SC_CHIP_ID_H		0x300a
+#define REG_SC_CHIP_ID_L		0x300b
+#define REG_SC_PWC			0x3014
+#define REG_SC_CLKRST0			0x301a
+#define REG_SC_CLKRST1			0x301b
+#define REG_SC_CLKRST2			0x301c
+#define REG_SC_CLKRST3			0x301d
+#define REG_SC_SUB_ID			0x302a
+#define REG_SC_SCCB_ID			0x302b
+
+#define REG_GROUP_ADDRESS_00		0x3200
+#define REG_GROUP_ADDRESS_01		0x3201
+#define REG_GROUP_ADDRESS_02		0x3202
+#define REG_GROUP_ADDRESS_03		0x3203
+#define REG_GROUP_ACCESS		0x3208
+
+#define REG_AWB_R_GAIN_H		0x3400
+#define REG_AWB_R_GAIN_L		0x3401
+#define REG_AWB_G_GAIN_H		0x3402
+#define REG_AWB_G_GAIN_L		0x3403
+#define REG_AWB_B_GAIN_H		0x3404
+#define REG_AWB_B_GAIN_L		0x3405
+#define REG_AWB_MANUAL_CONTROL		0x3406
+
+#define REG_TIMING_HS_H			0x3800
+#define REG_TIMING_HS_L			0x3801
+#define REG_TIMING_VS_H			0x3802
+#define REG_TIMING_VS_L			0x3803
+#define REG_TIMING_HW_H			0x3804
+#define REG_TIMING_HW_L			0x3805
+#define REG_TIMING_VH_H			0x3806
+#define REG_TIMING_VH_L			0x3807
+#define REG_TIMING_DVPHO_H		0x3808
+#define REG_TIMING_DVPHO_L		0x3809
+#define REG_TIMING_DVPVO_H		0x380a
+#define REG_TIMING_DVPVO_L		0x380b
+#define REG_TIMING_HTS_H		0x380c
+#define REG_TIMING_HTS_L		0x380d
+#define REG_TIMING_VTS_H		0x380e
+#define REG_TIMING_VTS_L		0x380f
+#define REG_TIMING_HOFFS_H		0x3810
+#define REG_TIMING_HOFFS_L		0x3811
+#define REG_TIMING_VOFFS_H		0x3812
+#define REG_TIMING_VOFFS_L		0x3813
+#define REG_TIMING_XINC			0x3814
+#define REG_TIMING_YINC			0x3815
+#define REG_TIMING_VERT_FORMAT		0x3820
+#define REG_TIMING_HORIZ_FORMAT		0x3821
+
+#define REG_AEC_PK_EXPOSURE_H		0x3500
+#define REG_AEC_PK_EXPOSURE_M		0x3501
+#define REG_AEC_PK_EXPOSURE_L		0x3502
+#define REG_AEC_PK_MANUAL		0x3503
+#define REG_AEC_MANUAL_GAIN_H		0x3504
+#define REG_AEC_MANUAL_GAIN_L		0x3505
+#define REG_AEC_ADD_VTS_H		0x3506
+#define REG_AEC_ADD_VTS_L		0x3507
+#define REG_AEK_PK_CTRL_08		0x3508
+#define REG_AEK_PK_CTRL_09		0x3509
+#define REG_AEC_PK_REAL_GAIN_H		0x350a
+#define REG_AEC_PK_REAL_GAIN_L		0x350b
+#define REG_AEC_REAL_GAIN_READ_H	0x3510
+#define REG_AEC_REAL_GAIN_READ_L	0x3511
+#define REG_AEC_SNR_GAIN_READ_H		0x3512
+#define REG_AEC_SNR_GAIN_READ_L		0x3513
+
+#define REG_AEC_CTRL(num)		(0x3a00 + num)
+
+#define REG_FRAME_CTRL00		0x4201
+#define REG_FRAME_CTRL01		0x4202
+#define REG_FORMAT_CTRL00		0x4300
+#define REG_CLIPPING_CTRL		0x4301
+
+#define REG_VFIFO_READ_CTRL		0x4601
+#define REG_VFIFO_CTRL05		0x4605
+#define REG_VFIFO_READ_START_H		0x4608
+#define REG_VFIFO_READ_START_L		0x4609
+
+#define REG_DVP_CTRL01			0x4704
+#define REG_DVP_CTRL02			0x4708
+#define REG_DVP_CTRL03			0x4709
+
+#define REG_ISP_CTRL00			0x5000
+#define REG_ISP_CTRL01			0x5001
+#define REG_ISP_CTRL02			0x5002
+#define REG_ISP_CTRL07			0x5007
+#define REG_ISP_PRE_CTRL00		0x50a0
+
+#define REG_BLC_CTRL00			0x4000
+#define REG_BLC_START_LINE		0x4001
+#define REG_BLC_CTRL02			0x4002
+#define REG_BLC_CTRL03			0x4003
+#define REG_BLC_LINE_NUM		0x4004
+#define REG_BLC_TARGET			0x4009
+
+#define REG_LENC_RED_X0_H		0x500c
+#define REG_LENC_RED_X0_L		0x500d
+#define REG_LENC_RED_Y0_H		0x500e
+#define REG_LENC_RED_Y0_L		0x500f
+#define REG_LENC_RED_A1			0x5010
+#define REG_LENC_RED_B1			0x5011
+#define REG_LENC_RED_A2_B2		0x5012
+#define REG_LENC_GREEN_X0_H		0x5013
+#define REG_LENC_GREEN_X0_L		0x5014
+#define REG_LENC_GREEN_Y0_H		0x5015
+#define REG_LENC_GREEN_Y0_L		0x5016
+#define REG_LENC_GREEN_A1		0x5017
+#define REG_LENC_GREEN_B1		0x5018
+#define REG_LENC_GREEN_A2_B2		0x5019
+#define REG_LENC_BLUE_X0_H		0x501a
+#define REG_LENC_BLUE_X0_L		0x501b
+#define REG_LENC_BLUE_Y0_H		0x501c
+#define REG_LENC_BLUE_Y0_L		0x501d
+#define REG_LENC_BLUE_A1		0x501e
+#define REG_LENC_BLUE_B1		0x501f
+#define REG_LENC_BLUE_A2_B2		0x5020
+#define REG_LENC_CTRL00			0x5021
+#define REG_LENC_CTRL01			0x5022
+#define REG_COEFFICIENT_THRESH		0x5023
+#define REG_COEFFICIENT_MANUAL_VAL	0x5024
+
+#define REG_GAMMA_YST(num)		(0x5025 + (num - 1))
+#define REG_GAMMA_YSLP			0x5034
+
+#define REG_AWB_CTRL00			0x5035
+#define REG_AWB_CTRL01			0x5036
+#define REG_AWB_CTRL02			0x5037
+#define REG_AWB_CTRL03			0x5038
+#define REG_AWB_CTRL04			0x5039
+#define REG_AWB_LOCAL_LIMIT		0x503a
+#define REG_AWB_CTRL12			0x5049
+#define REG_AWB_CTRL13			0x504a
+#define REG_AWB_CTRL14			0x504b
+
+#define REG_AVG_CTRL(num)		(0x5060 + num)
+#define REG_AVG_READOUT			0x5237
+
+#define REG_SHARPENMT_THRESH1		0x5064
+#define REG_SHARPENMT_THRESH2		0x5065
+#define REG_SHARPENMT_OFFSET1		0x5066
+#define REG_SHARPENMT_OFFSET2		0x5067
+#define REG_DENOISE_THRESH1		0x5068
+#define REG_DENOISE_THRESH2		0x5069
+#define REG_DENOISE_OFFSET1		0x506a
+#define REG_DENOISE_OFFSET2		0x506b
+#define REG_SHARPEN_THRESH1		0x506c
+#define REG_SHARPEN_THRESH2		0x506d
+#define REG_CIP_CTRL00			0x506e
+#define REG_CIP_CTRL01			0x506f
+
+#define REG_CMX(num)			(0x5070 + (num - 1))
+#define REG_CMX_SIGN			0x5079
+#define REG_CMX_MISC_CTRL		0x507a
+
+#define REG_SDE_CTRL(num)		(0x507b + num)
+
+#define REG_SCALE_CTRL0			0x5600
+#define REG_SCALE_CTRL1			0x5601
+#define REG_XSC_H			0x5602
+#define REG_XSC_L			0x5603
+#define REG_YSC_H			0x5604
+#define REG_YSC_L			0x5605
+#define REG_VOFFSET			0x5606
+#define REG_NULL			0x0000	/* Array end token */
+
+#define OV265X_ID(_msb, _lsb)		((_msb) << 8 | (_lsb))
+#define OV2659_ID			0x2656
+
+#define OV2659_DEFAULT_MCLK_FREQ	12000000
+
+/* Target Pixel Clock */
+#define OV2659_PIXEL_CLOCK		70000000
+
+struct sensor_register {
+	u16 addr;
+	u8 value;
+};
+
+struct ov2659_framesize {
+	u16 width;
+	u16 height;
+	u16 max_exp_lines;
+	const struct sensor_register *regs;
+};
+
+struct ov2659_pll_ctrl {
+	u8 ctrl1;
+	u8 ctrl2;
+	u8 ctrl3;
+};
+
+struct ov2659_pixfmt {
+	u32 code;
+	u32 colorspace;
+	/* Output format Register Value (REG_FORMAT_CTRL00) */
+	struct sensor_register *format_ctrl_regs;
+};
+
+struct pll_ctrl_reg {
+	unsigned int div;
+	unsigned char reg;
+};
+
+struct ov2659 {
+	struct v4l2_subdev sd;
+	struct media_pad pad;
+	enum v4l2_mbus_type bus_type;
+	const struct ov2659_platform_data *pdata;
+
+	/* Protects the struct fields below */
+	struct mutex lock;
+
+	struct i2c_client *client;
+
+	unsigned short id;
+	const struct ov2659_framesize *frame_size;
+	/* Current Output format Register Value (REG_FORMAT_CTRL00) */
+	struct sensor_register *format_ctrl_regs;
+
+	struct v4l2_mbus_framefmt format;
+
+	/* Sensor specific feq/pll config */
+	struct ov2659_pll_ctrl pll;
+
+	int streaming;
+	int power;
+};
+
+static const struct sensor_register ov2659_init_regs[] = {
+	{0x3000, 0x03}, /* IO CTRL */
+	{0x3001, 0xff}, /* IO CTRL */
+	{0x3002, 0xe0}, /* IO CTRL */
+	{0x3633, 0x3d},
+	{0x3620, 0x02},
+	{0x3631, 0x11},
+	{0x3612, 0x04},
+	{0x3630, 0x20},
+	{0x4702, 0x02}, /* DVP Debug mode */
+	{0x370c, 0x34},
+	{0x3004, 0x10}, /* System Divider */
+	{0x3005, 0x22}, /* Pixel clock Multiplier */
+	{0x3800, 0x00}, /* TIMING */
+	{0x3801, 0x00}, /* TIMING */
+	{0x3802, 0x00}, /* TIMING */
+	{0x3803, 0x00}, /* TIMING */
+	{0x3804, 0x06}, /* TIMING */
+	{0x3805, 0x5f}, /* TIMING */
+	{0x3806, 0x04}, /* TIMING */
+	{0x3807, 0xb7}, /* TIMING */
+	{0x3808, 0x03}, /* Horizontal High Byte */
+	{0x3809, 0x20}, /* Horizontal Low Byte */
+	{0x380a, 0x02}, /* Vertical High Byte */
+	{0x380b, 0x58}, /* Vertical Low Byte */
+	{0x380c, 0x05}, /* TIMING */
+	{0x380d, 0x14}, /* TIMING */
+	{0x380e, 0x02}, /* TIMING */
+	{0x380f, 0x68}, /* TIMING */
+	{0x3811, 0x08}, /* TIMING */
+	{0x3813, 0x02}, /* TIMING */
+	{0x3814, 0x31}, /* TIMING */
+	{0x3815, 0x31}, /* TIMING */
+	{0x3a02, 0x02}, /* AEC */
+	{0x3a03, 0x68}, /* AEC */
+	{0x3a08, 0x00}, /* AEC */
+	{0x3a09, 0x5c}, /* AEC */
+	{0x3a0a, 0x00}, /* AEC */
+	{0x3a0b, 0x4d}, /* AEC */
+	{0x3a0d, 0x08}, /* AEC */
+	{0x3a0e, 0x06}, /* AEC */
+	{0x3a14, 0x02}, /* AEC */
+	{0x3a15, 0x28}, /* AEC */
+	{0x4708, 0x01}, /* DVP */
+	{0x3623, 0x00},
+	{0x3634, 0x76},
+	{0x3701, 0x44},
+	{0x3702, 0x18},
+	{0x3703, 0x24},
+	{0x3704, 0x24},
+	{0x3705, 0x0c},
+	{0x3820, 0x81}, /* TIMING */
+	{0x3821, 0x01}, /* TIMING */
+	{0x370a, 0x52},
+	{0x4608, 0x00}, /* VFIFO */
+	{0x4609, 0x80}, /* VFIFO */
+	{0x4300, 0x30}, /* Format */
+	{0x5086, 0x02},
+	{0x5000, 0xfb}, /* DPC/ISP */
+	{0x5001, 0x1f}, /* ISP */
+	{0x5002, 0x00}, /* ISP */
+	{0x5025, 0x0e}, /* GAMMA */
+	{0x5026, 0x18}, /* GAMMA */
+	{0x5027, 0x34}, /* GAMMA */
+	{0x5028, 0x4c}, /* GAMMA */
+	{0x5029, 0x62}, /* GAMMA */
+	{0x502a, 0x74}, /* GAMMA */
+	{0x502b, 0x85}, /* GAMMA */
+	{0x502c, 0x92}, /* GAMMA */
+	{0x502d, 0x9e}, /* GAMMA */
+	{0x502e, 0xb2}, /* GAMMA */
+	{0x502f, 0xc0}, /* GAMMA */
+	{0x5030, 0xcc}, /* GAMMA */
+	{0x5031, 0xe0}, /* GAMMA */
+	{0x5032, 0xee}, /* GAMMA */
+	{0x5033, 0xf6}, /* GAMMA */
+	{0x5034, 0x11}, /* GAMMA */
+	{0x5070, 0x1c}, /* CMX */
+	{0x5071, 0x5b}, /* CMX */
+	{0x5072, 0x05}, /* CMX */
+	{0x5073, 0x20}, /* CMX */
+	{0x5074, 0x94}, /* CMX */
+	{0x5075, 0xb4}, /* CMX */
+	{0x5076, 0xb4}, /* CMX */
+	{0x5077, 0xaf}, /* CMX */
+	{0x5078, 0x05}, /* CMX */
+	{0x5079, 0x98}, /* CMX */
+	{0x507a, 0x21}, /* CMX */
+	{0x5035, 0x6a}, /* AWB */
+	{0x5036, 0x11}, /* AWB */
+	{0x5037, 0x92}, /* AWB */
+	{0x5038, 0x21}, /* AWB */
+	{0x5039, 0xe1}, /* AWB */
+	{0x503a, 0x01}, /* AWB */
+	{0x503c, 0x05}, /* AWB */
+	{0x503d, 0x08}, /* AWB */
+	{0x503e, 0x08}, /* AWB */
+	{0x503f, 0x64}, /* AWB */
+	{0x5040, 0x58}, /* AWB */
+	{0x5041, 0x2a}, /* AWB */
+	{0x5042, 0xc5}, /* AWB */
+	{0x5043, 0x2e}, /* AWB */
+	{0x5044, 0x3a}, /* AWB */
+	{0x5045, 0x3c}, /* AWB */
+	{0x5046, 0x44}, /* AWB */
+	{0x5047, 0xf8}, /* AWB */
+	{0x5048, 0x08}, /* AWB */
+	{0x5049, 0x70}, /* AWB */
+	{0x504a, 0xf0}, /* AWB */
+	{0x504b, 0xf0}, /* AWB */
+	{0x500c, 0x03}, /* LENC/ISP TOP Debug */
+	{0x500d, 0x20}, /* LENC/ISP TOP Debug */
+	{0x500e, 0x02}, /* LENC/ISP TOP Debug */
+	{0x500f, 0x5c}, /* LENC/ISP TOP Debug */
+	{0x5010, 0x48}, /* LENC/ISP TOP Debug */
+	{0x5011, 0x00}, /* LENC/ISP TOP Debug */
+	{0x5012, 0x66}, /* LENC/ISP TOP Debug */
+	{0x5013, 0x03}, /* LENC/ISP TOP Debug */
+	{0x5014, 0x30}, /* LENC/ISP TOP Debug */
+	{0x5015, 0x02}, /* LENC/ISP TOP Debug */
+	{0x5016, 0x7c}, /* LENC/ISP TOP Debug */
+	{0x5017, 0x40}, /* LENC/ISP TOP Debug */
+	{0x5018, 0x00}, /* LENC/ISP TOP Debug */
+	{0x5019, 0x66}, /* LENC/ISP TOP Debug */
+	{0x501a, 0x03}, /* LENC/ISP TOP Debug */
+	{0x501b, 0x10}, /* LENC/ISP TOP Debug */
+	{0x501c, 0x02}, /* LENC/ISP TOP Debug */
+	{0x501d, 0x7c}, /* LENC/ISP TOP Debug */
+	{0x501e, 0x3a}, /* LENC/ISP TOP Debug */
+	{0x501f, 0x00}, /* LENC/ISP TOP Debug */
+	{0x5020, 0x66}, /* LENC/ISP TOP Debug */
+	{0x506e, 0x44}, /* CIP/DNS */
+	{0x5064, 0x08}, /* CIP/DNS */
+	{0x5065, 0x10}, /* CIP/DNS */
+	{0x5066, 0x12}, /* CIP/DNS */
+	{0x5067, 0x02}, /* CIP/DNS */
+	{0x506c, 0x08}, /* CIP/DNS */
+	{0x506d, 0x10}, /* CIP/DNS */
+	{0x506f, 0xa6}, /* CIP/DNS */
+	{0x5068, 0x08}, /* CIP/DNS */
+	{0x5069, 0x10}, /* CIP/DNS */
+	{0x506a, 0x04}, /* CIP/DNS */
+	{0x506b, 0x12}, /* CIP/DNS */
+	{0x507e, 0x40}, /* SDE */
+	{0x507f, 0x20}, /* SDE */
+	{0x507b, 0x02}, /* SDE */
+	{0x507a, 0x01}, /* CMX/SDE */
+	{0x5084, 0x0c}, /* SDE */
+	{0x5085, 0x3e}, /* SDE */
+	{0x5005, 0x80}, /* ISP TOP Debug */
+	{0x3a0f, 0x30}, /* AEC */
+	{0x3a10, 0x28}, /* AEC */
+	{0x3a1b, 0x32}, /* AEC */
+	{0x3a1e, 0x26}, /* AEC */
+	{0x3a11, 0x60}, /* AEC */
+	{0x3a1f, 0x14}, /* AEC */
+	{0x5060, 0x69}, /* Y AVG */
+	{0x5061, 0x7d}, /* Y AVG */
+	{0x5062, 0x7d}, /* Y AVG */
+	{0x5063, 0x69}, /* Y AVG */
+	{0x0000, 0x00}
+};
+
+/* 1280X720 720p */
+static struct sensor_register ov2659_720p[] = {
+	{0x3800, 0x00},
+	{0x3801, 0xa0},
+	{0x3802, 0x00},
+	{0x3803, 0xf0},
+	{0x3804, 0x05},
+	{0x3805, 0xbf},
+	{0x3806, 0x03},
+	{0x3807, 0xcb},
+	{0x3808, 0x05},
+	{0x3809, 0x00},
+	{0x380a, 0x02},
+	{0x380b, 0xd0},
+	{0x380c, 0x06},
+	{0x380d, 0x4c},
+	{0x380e, 0x02},
+	{0x380f, 0xe8},
+	{0x3811, 0x10},
+	{0x3813, 0x06},
+	{0x3814, 0x11},
+	{0x3815, 0x11},
+	{0x3820, 0x80},
+	{0x3821, 0x00},
+	{0x3a03, 0xe8},
+	{0x3a09, 0x6f},
+	{0x3a0b, 0x5d},
+	{0x3a15, 0x9a},
+	{0x0000, 0x00}
+};
+
+/* 1600X1200 UXGA */
+static struct sensor_register ov2659_uxga[] = {
+	{0x3800, 0x00},
+	{0x3801, 0x00},
+	{0x3802, 0x00},
+	{0x3803, 0x00},
+	{0x3804, 0x06},
+	{0x3805, 0x5f},
+	{0x3806, 0x04},
+	{0x3807, 0xbb},
+	{0x3808, 0x06},
+	{0x3809, 0x40},
+	{0x380a, 0x04},
+	{0x380b, 0xb0},
+	{0x380c, 0x07},
+	{0x380d, 0x9f},
+	{0x380e, 0x04},
+	{0x380f, 0xd0},
+	{0x3811, 0x10},
+	{0x3813, 0x06},
+	{0x3814, 0x11},
+	{0x3815, 0x11},
+	{0x3a02, 0x04},
+	{0x3a03, 0xd0},
+	{0x3a08, 0x00},
+	{0x3a09, 0xb8},
+	{0x3a0a, 0x00},
+	{0x3a0b, 0x9a},
+	{0x3a0d, 0x08},
+	{0x3a0e, 0x06},
+	{0x3a14, 0x04},
+	{0x3a15, 0x50},
+	{0x3623, 0x00},
+	{0x3634, 0x44},
+	{0x3701, 0x44},
+	{0x3702, 0x30},
+	{0x3703, 0x48},
+	{0x3704, 0x48},
+	{0x3705, 0x18},
+	{0x3820, 0x80},
+	{0x3821, 0x00},
+	{0x370a, 0x12},
+	{0x4608, 0x00},
+	{0x4609, 0x80},
+	{0x5002, 0x00},
+	{0x0000, 0x00}
+};
+
+/* 1280X1024 SXGA */
+static struct sensor_register ov2659_sxga[] = {
+	{0x3800, 0x00},
+	{0x3801, 0x00},
+	{0x3802, 0x00},
+	{0x3803, 0x00},
+	{0x3804, 0x06},
+	{0x3805, 0x5f},
+	{0x3806, 0x04},
+	{0x3807, 0xb7},
+	{0x3808, 0x05},
+	{0x3809, 0x00},
+	{0x380a, 0x04},
+	{0x380b, 0x00},
+	{0x380c, 0x07},
+	{0x380d, 0x9c},
+	{0x380e, 0x04},
+	{0x380f, 0xd0},
+	{0x3811, 0x10},
+	{0x3813, 0x06},
+	{0x3814, 0x11},
+	{0x3815, 0x11},
+	{0x3a02, 0x02},
+	{0x3a03, 0x68},
+	{0x3a08, 0x00},
+	{0x3a09, 0x5c},
+	{0x3a0a, 0x00},
+	{0x3a0b, 0x4d},
+	{0x3a0d, 0x08},
+	{0x3a0e, 0x06},
+	{0x3a14, 0x02},
+	{0x3a15, 0x28},
+	{0x3623, 0x00},
+	{0x3634, 0x76},
+	{0x3701, 0x44},
+	{0x3702, 0x18},
+	{0x3703, 0x24},
+	{0x3704, 0x24},
+	{0x3705, 0x0c},
+	{0x3820, 0x80},
+	{0x3821, 0x00},
+	{0x370a, 0x52},
+	{0x4608, 0x00},
+	{0x4609, 0x80},
+	{0x5002, 0x00},
+	{0x0000, 0x00}
+};
+
+/* 1024X768 SXGA */
+static struct sensor_register ov2659_xga[] = {
+	{0x3800, 0x00},
+	{0x3801, 0x00},
+	{0x3802, 0x00},
+	{0x3803, 0x00},
+	{0x3804, 0x06},
+	{0x3805, 0x5f},
+	{0x3806, 0x04},
+	{0x3807, 0xb7},
+	{0x3808, 0x04},
+	{0x3809, 0x00},
+	{0x380a, 0x03},
+	{0x380b, 0x00},
+	{0x380c, 0x07},
+	{0x380d, 0x9c},
+	{0x380e, 0x04},
+	{0x380f, 0xd0},
+	{0x3811, 0x10},
+	{0x3813, 0x06},
+	{0x3814, 0x11},
+	{0x3815, 0x11},
+	{0x3a02, 0x02},
+	{0x3a03, 0x68},
+	{0x3a08, 0x00},
+	{0x3a09, 0x5c},
+	{0x3a0a, 0x00},
+	{0x3a0b, 0x4d},
+	{0x3a0d, 0x08},
+	{0x3a0e, 0x06},
+	{0x3a14, 0x02},
+	{0x3a15, 0x28},
+	{0x3623, 0x00},
+	{0x3634, 0x76},
+	{0x3701, 0x44},
+	{0x3702, 0x18},
+	{0x3703, 0x24},
+	{0x3704, 0x24},
+	{0x3705, 0x0c},
+	{0x3820, 0x80},
+	{0x3821, 0x00},
+	{0x370a, 0x52},
+	{0x4608, 0x00},
+	{0x4609, 0x80},
+	{0x5002, 0x00},
+	{0x0000, 0x00}
+};
+
+/* 800X600 SVGA */
+static struct sensor_register ov2659_svga[] = {
+	{0x3800, 0x00},
+	{0x3801, 0x00},
+	{0x3802, 0x00},
+	{0x3803, 0x00},
+	{0x3804, 0x06},
+	{0x3805, 0x5f},
+	{0x3806, 0x04},
+	{0x3807, 0xb7},
+	{0x3808, 0x03},
+	{0x3809, 0x20},
+	{0x380a, 0x02},
+	{0x380b, 0x58},
+	{0x380c, 0x05},
+	{0x380d, 0x14},
+	{0x380e, 0x02},
+	{0x380f, 0x68},
+	{0x3811, 0x08},
+	{0x3813, 0x02},
+	{0x3814, 0x31},
+	{0x3815, 0x31},
+	{0x3a02, 0x02},
+	{0x3a03, 0x68},
+	{0x3a08, 0x00},
+	{0x3a09, 0x5c},
+	{0x3a0a, 0x00},
+	{0x3a0b, 0x4d},
+	{0x3a0d, 0x08},
+	{0x3a0e, 0x06},
+	{0x3a14, 0x02},
+	{0x3a15, 0x28},
+	{0x3623, 0x00},
+	{0x3634, 0x76},
+	{0x3701, 0x44},
+	{0x3702, 0x18},
+	{0x3703, 0x24},
+	{0x3704, 0x24},
+	{0x3705, 0x0c},
+	{0x3820, 0x81},
+	{0x3821, 0x01},
+	{0x370a, 0x52},
+	{0x4608, 0x00},
+	{0x4609, 0x80},
+	{0x5002, 0x00},
+	{0x0000, 0x00}
+};
+
+/* 640X480 VGA */
+static struct sensor_register ov2659_vga[] = {
+	{0x3800, 0x00},
+	{0x3801, 0x00},
+	{0x3802, 0x00},
+	{0x3803, 0x00},
+	{0x3804, 0x06},
+	{0x3805, 0x5f},
+	{0x3806, 0x04},
+	{0x3807, 0xb7},
+	{0x3808, 0x02},
+	{0x3809, 0x80},
+	{0x380a, 0x01},
+	{0x380b, 0xe0},
+	{0x380c, 0x05},
+	{0x380d, 0x14},
+	{0x380e, 0x02},
+	{0x380f, 0x68},
+	{0x3811, 0x08},
+	{0x3813, 0x02},
+	{0x3814, 0x31},
+	{0x3815, 0x31},
+	{0x3a02, 0x02},
+	{0x3a03, 0x68},
+	{0x3a08, 0x00},
+	{0x3a09, 0x5c},
+	{0x3a0a, 0x00},
+	{0x3a0b, 0x4d},
+	{0x3a0d, 0x08},
+	{0x3a0e, 0x06},
+	{0x3a14, 0x02},
+	{0x3a15, 0x28},
+	{0x3623, 0x00},
+	{0x3634, 0x76},
+	{0x3701, 0x44},
+	{0x3702, 0x18},
+	{0x3703, 0x24},
+	{0x3704, 0x24},
+	{0x3705, 0x0c},
+	{0x3820, 0x81},
+	{0x3821, 0x01},
+	{0x370a, 0x52},
+	{0x4608, 0x00},
+	{0x4609, 0x80},
+	{0x5002, 0x10},
+	{0x0000, 0x00}
+};
+
+/* 320X240 QVGA */
+static  struct sensor_register ov2659_qvga[] = {
+	{0x3800, 0x00},
+	{0x3801, 0x00},
+	{0x3802, 0x00},
+	{0x3803, 0x00},
+	{0x3804, 0x06},
+	{0x3805, 0x5f},
+	{0x3806, 0x04},
+	{0x3807, 0xb7},
+	{0x3808, 0x01},
+	{0x3809, 0x40},
+	{0x380a, 0x00},
+	{0x380b, 0xf0},
+	{0x380c, 0x05},
+	{0x380d, 0x14},
+	{0x380e, 0x02},
+	{0x380f, 0x68},
+	{0x3811, 0x08},
+	{0x3813, 0x02},
+	{0x3814, 0x31},
+	{0x3815, 0x31},
+	{0x3a02, 0x02},
+	{0x3a03, 0x68},
+	{0x3a08, 0x00},
+	{0x3a09, 0x5c},
+	{0x3a0a, 0x00},
+	{0x3a0b, 0x4d},
+	{0x3a0d, 0x08},
+	{0x3a0e, 0x06},
+	{0x3a14, 0x02},
+	{0x3a15, 0x28},
+	{0x3623, 0x00},
+	{0x3634, 0x76},
+	{0x3701, 0x44},
+	{0x3702, 0x18},
+	{0x3703, 0x24},
+	{0x3704, 0x24},
+	{0x3705, 0x0c},
+	{0x3820, 0x81},
+	{0x3821, 0x01},
+	{0x370a, 0x52},
+	{0x4608, 0x00},
+	{0x4609, 0x80},
+	{0x5002, 0x10},
+	{0x0000, 0x00}
+};
+
+static const struct pll_ctrl_reg ctrl3[] = {
+	{1, 0x00},
+	{2, 0x02},
+	{3, 0x03},
+	{4, 0x06},
+	{6, 0x0d},
+	{8, 0x0e},
+	{12, 0x0f},
+	{16, 0x12},
+	{24, 0x13},
+	{32, 0x16},
+	{48, 0x1b},
+	{64, 0x1e},
+	{96, 0x1f},
+	{0, 0x00},
+};
+
+static const struct pll_ctrl_reg ctrl1[] = {
+	{2, 0x10},
+	{4, 0x20},
+	{6, 0x30},
+	{8, 0x40},
+	{10, 0x50},
+	{12, 0x60},
+	{14, 0x70},
+	{16, 0x80},
+	{18, 0x90},
+	{20, 0xa0},
+	{22, 0xb0},
+	{24, 0xc0},
+	{26, 0xd0},
+	{28, 0xe0},
+	{30, 0xf0},
+	{0, 0x00},
+};
+
+static const struct ov2659_framesize ov2659_framesizes[] = {
+	{ /* QVGA */
+		.width		= 320,
+		.height		= 240,
+		.regs		= ov2659_qvga,
+		.max_exp_lines	= 248,
+	}, { /* VGA */
+		.width		= 640,
+		.height		= 480,
+		.regs		= ov2659_vga,
+		.max_exp_lines	= 498,
+	}, { /* SVGA */
+		.width		= 800,
+		.height		= 600,
+		.regs		= ov2659_svga,
+		.max_exp_lines	= 498,
+	}, { /* XGA */
+		.width		= 1024,
+		.height		= 768,
+		.regs		= ov2659_xga,
+		.max_exp_lines	= 498,
+	}, { /* 720P */
+		.width		= 1280,
+		.height		= 720,
+		.regs		= ov2659_720p,
+		.max_exp_lines	= 498,
+	}, { /* SXGA */
+		.width		= 1280,
+		.height		= 1024,
+		.regs		= ov2659_sxga,
+		.max_exp_lines	= 1048,
+	}, { /* UXGA */
+		.width		= 1600,
+		.height		= 1200,
+		.regs		= ov2659_uxga,
+		.max_exp_lines	= 498,
+	},
+};
+
+/* YUV422 YUYV*/
+static struct sensor_register ov2659_format_yuyv[] = {
+	{0x4300, 0x30},
+	{0x0000, 0x0},
+};
+
+/* YUV422 UYVY  */
+static struct sensor_register ov2659_format_uyvy[] = {
+	{0x4300, 0x32},
+	{0x0000, 0x0},
+};
+
+/* Raw Bayer BGGR */
+static struct sensor_register ov2659_format_bggr[] = {
+	{0x4300, 0x00},
+	{0x0000, 0x0}
+};
+
+/* RGB565 */
+static struct sensor_register ov2659_format_rgb565[] = {
+	{0x4300, 0x60},
+	{0x0000, 0x0},
+};
+
+static const struct ov2659_pixfmt ov2659_formats[] = {
+	{
+		.code = MEDIA_BUS_FMT_YUYV8_2X8,
+		.colorspace = V4L2_COLORSPACE_JPEG,
+		.format_ctrl_regs = ov2659_format_yuyv,
+	},
+	{
+		.code = MEDIA_BUS_FMT_UYVY8_2X8,
+		.colorspace = V4L2_COLORSPACE_JPEG,
+		.format_ctrl_regs = ov2659_format_uyvy,
+	},
+	{
+		.code = MEDIA_BUS_FMT_RGB565_2X8_BE,
+		.colorspace = V4L2_COLORSPACE_JPEG,
+		.format_ctrl_regs = ov2659_format_rgb565,
+	},
+	{
+		.code = MEDIA_BUS_FMT_SBGGR8_1X8,
+		.colorspace = V4L2_COLORSPACE_SMPTE170M,
+		.format_ctrl_regs = ov2659_format_bggr,
+	},
+};
+
+static inline struct ov2659 *to_ov2659(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct ov2659, sd);
+}
+
+/* sensor register write */
+static int ov2659_write(struct i2c_client *client, u16 reg, u8 val)
+{
+	struct i2c_msg msg[1];
+	int ret, i;
+	u8 buf[3];
+
+	buf[0] = reg >> 8;
+	buf[1] = reg & 0xFF;
+	buf[2] = val;
+
+	msg->addr = client->addr;
+	msg->flags = client->flags;
+	msg->buf = buf;
+	msg->len = sizeof(buf);
+
+	for (i = 0; i < 5; i++) {
+		ret = i2c_transfer(client->adapter, msg, 1);
+		if (ret >= 0)
+			return 0;
+
+		dev_dbg(&client->dev,
+			"ov2659 write reg(0x%x val:0x%x) failed !\n", reg, val);
+		udelay(10);
+	}
+
+	return ret;
+}
+
+/* sensor register read */
+static int ov2659_read(struct i2c_client *client, u16 reg, u8 *val)
+{
+	struct i2c_msg msg[2];
+	int ret, i;
+	u8 buf[2];
+
+	buf[0] = reg >> 8;
+	buf[1] = reg & 0xFF;
+
+	msg[0].addr = client->addr;
+	msg[0].flags = client->flags;
+	msg[0].buf = buf;
+	msg[0].len = sizeof(buf);
+
+	msg[1].addr = client->addr;
+	msg[1].flags = client->flags|I2C_M_RD;
+	msg[1].buf = buf;
+	msg[1].len = 1;
+
+	for (i = 0; i < 5; i++) {
+		ret = i2c_transfer(client->adapter, msg, 2);
+		if (ret >= 0) {
+			*val = buf[0];
+			return 0;
+		}
+
+		dev_dbg(&client->dev,
+			"ov2659 read reg(0x%x val:0x%x) failed !\n", reg, *val);
+		udelay(10);
+	}
+
+	return ret;
+}
+
+static int ov2659_write_array(struct i2c_client *client,
+			      const struct sensor_register *regs)
+{
+	int i, ret = 0;
+
+	for (i = 0; ret == 0 && regs[i].addr; i++) {
+		ret = ov2659_write(client, regs[i].addr, regs[i].value);
+		usleep_range(5000, 6000);
+	}
+
+	return ret;
+}
+
+static int dump_reg(struct i2c_client *client, u16 reg)
+{
+	u8 val = 0;
+	int ret;
+
+	ret = ov2659_read(client, reg, &val);
+	dev_dbg(&client->dev, "%s: 0x%04x: 0x%02x\n",
+				__func__, reg, val);
+	return ret;
+}
+
+static void ov2659_reg_dump(struct i2c_client *client)
+{
+	int i;
+
+	dump_reg(client, REG_SOFTWARE_STANDBY);
+	dump_reg(client, REG_SOFTWARE_RESET);
+
+	for (i = 0x3000; i <= 0x302f; i++)
+		dump_reg(client, i);
+
+	for (i = 0x3400; i <= 0x3406; i++)
+		dump_reg(client, i);
+
+	for (i = 0x3500; i <= 0x3513; i++)
+		dump_reg(client, i);
+
+	for (i = 0x3600; i <= 0x3640; i++)
+		dump_reg(client, i);
+
+	for (i = 0x3800; i <= 0x3821; i++)
+		dump_reg(client, i);
+
+	for (i = 0x3a00; i <= 0x3a26; i++)
+		dump_reg(client, i);
+
+	for (i = 0x4000; i <= 0x4009; i++)
+		dump_reg(client, i);
+
+	for (i = 0x4201; i <= 0x4202; i++)
+		dump_reg(client, i);
+
+	for (i = 0x4300; i <= 0x4301; i++)
+		dump_reg(client, i);
+
+	for (i = 0x4600; i <= 0x4609; i++)
+		dump_reg(client, i);
+
+	for (i = 0x4700; i <= 0x4709; i++)
+		dump_reg(client, i);
+
+	for (i = 0x5000; i <= 0x50a0; i++)
+		dump_reg(client, i);
+
+	for (i = 0x5600; i <= 0x5606; i++)
+		dump_reg(client, i);
+}
+
+static unsigned int ov2659_pll_calc_params(struct ov2659 *ov2659)
+{
+	const struct ov2659_platform_data *pdata = ov2659->pdata;
+	u8 ctrl1_reg = 0, ctrl2_reg = 0, ctrl3_reg = 0;
+	struct i2c_client *client = ov2659->client;
+	u32 s_prediv = 1, s_postdiv = 1, s_mult = 1;
+	u32 desired = OV2659_PIXEL_CLOCK;
+	u32 prediv, postdiv, mult;
+	u32 bestdelta = -1;
+	u32 delta, actual;
+	int i, j;
+
+	for (i = 0; ctrl1[i].div != 0; i++) {
+		postdiv = ctrl1[i].div;
+		for (j = 0; ctrl3[j].div != 0; j++) {
+			prediv = ctrl3[j].div;
+			for (mult = 1; mult <= 63; mult++) {
+				actual  = pdata->mclk_frequency;
+				actual *= mult;
+				actual /= prediv;
+				actual /= postdiv;
+				delta = (actual-desired);
+				delta = abs(delta);
+
+				if ((delta < bestdelta) || (bestdelta == -1)) {
+					bestdelta = delta;
+					s_mult    = mult;
+					s_prediv  = prediv;
+					s_postdiv = postdiv;
+					ctrl1_reg = ctrl1[i].reg;
+					ctrl2_reg = mult;
+					ctrl3_reg = ctrl3[j].reg;
+				}
+			}
+		}
+	}
+	actual = pdata->mclk_frequency * (s_mult);
+	actual /= (s_prediv) * (s_postdiv);
+
+	dev_dbg(&client->dev, "Actual osc: %u pixel_clock: %u\n",
+		pdata->mclk_frequency, actual);
+
+	ov2659->pll.ctrl1 = ctrl1_reg;
+	ov2659->pll.ctrl2 = ctrl2_reg;
+	ov2659->pll.ctrl3 = ctrl3_reg;
+
+	dev_dbg(&client->dev,
+		"Actual reg config: ctrl1_reg: %02x ctrl2_reg: %02x ctrl3_reg: %02x\n",
+		ctrl1_reg, ctrl2_reg, ctrl3_reg);
+
+	return actual;
+}
+
+static int ov2659_set_pixel_clock(struct ov2659 *ov2659)
+{
+	struct i2c_client *client = ov2659->client;
+	struct sensor_register pll_regs[] = {
+		{0x3004, 0x10}, /* System Divider */
+		{0x3005, 0x22}, /* Pixel clock Multiplier */
+		{0x3006, 0x0d}, /* System Divider */
+		{0x0000, 0x00},
+	};
+
+	pll_regs[0].value = ov2659->pll.ctrl1;
+	pll_regs[1].value = ov2659->pll.ctrl2;
+	pll_regs[2].value = ov2659->pll.ctrl3;
+
+	dev_dbg(&client->dev, "%s\n", __func__);
+
+	return ov2659_write_array(client, pll_regs);
+};
+
+static void ov2659_get_default_format(struct v4l2_mbus_framefmt *format)
+{
+	format->width = ov2659_framesizes[2].width;
+	format->height = ov2659_framesizes[2].height;
+	format->colorspace = ov2659_formats[0].colorspace;
+	format->code = ov2659_formats[0].code;
+	format->field = V4L2_FIELD_NONE;
+}
+
+static void ov2659_set_streaming(struct ov2659 *ov2659, int on)
+{
+	struct i2c_client *client = ov2659->client;
+	int ret;
+
+	on = !!on;
+
+	dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
+
+	ret = ov2659_write(client, REG_SOFTWARE_STANDBY, on);
+	if (ret)
+		dev_err(&client->dev, "ov2659 soft standby failed\n");
+}
+
+static void __ov2659_set_power(struct ov2659 *ov2659, int on)
+{
+	struct i2c_client *client = ov2659->client;
+
+	on = !!on;
+
+	dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
+
+	if (ov2659->power == on)
+		return;
+
+	ov2659->power = on;
+}
+
+static int ov2659_s_power(struct v4l2_subdev *sd, int on)
+{
+	struct ov2659 *ov2659 = to_ov2659(sd);
+	struct i2c_client *client = ov2659->client;
+
+	dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
+
+	mutex_lock(&ov2659->lock);
+	__ov2659_set_power(ov2659, on);
+	mutex_unlock(&ov2659->lock);
+
+	return 0;
+}
+
+/*
+ * V4L2 subdev video and pad level operations
+ */
+
+static int ov2659_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_fh *fh,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	dev_dbg(&client->dev, "%s:\n", __func__);
+
+	if (code->index >= ARRAY_SIZE(ov2659_formats))
+		return -EINVAL;
+
+	code->code = ov2659_formats[code->index].code;
+
+	return 0;
+}
+
+static int ov2659_enum_frame_sizes(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_fh *fh,
+				   struct v4l2_subdev_frame_size_enum *fse)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	int i = ARRAY_SIZE(ov2659_formats);
+
+	dev_dbg(&client->dev, "%s:\n", __func__);
+
+	if (fse->index >= ARRAY_SIZE(ov2659_framesizes))
+		return -EINVAL;
+
+	while (--i)
+		if (fse->code == ov2659_formats[i].code)
+			break;
+
+	fse->code = ov2659_formats[i].code;
+
+	fse->min_width  = ov2659_framesizes[fse->index].width;
+	fse->max_width  = fse->min_width;
+	fse->max_height = ov2659_framesizes[fse->index].height;
+	fse->min_height = fse->max_height;
+
+	return 0;
+}
+
+static int ov2659_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			  struct v4l2_subdev_format *fmt)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ov2659 *ov2659 = to_ov2659(sd);
+	struct v4l2_mbus_framefmt *mf;
+
+	dev_dbg(&client->dev, "ov2659_get_fmt\n");
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+		mf = v4l2_subdev_get_try_format(fh, 0);
+		fmt->format = *mf;
+		return 0;
+	}
+
+	mutex_lock(&ov2659->lock);
+	fmt->format = ov2659->format;
+	mutex_unlock(&ov2659->lock);
+
+	dev_dbg(&client->dev, "ov2659_get_fmt: %x %dx%d\n",
+		ov2659->format.code, ov2659->format.width,
+		ov2659->format.height);
+
+	return 0;
+}
+
+static void __ov2659_try_frame_size(struct v4l2_mbus_framefmt *mf,
+				    const struct ov2659_framesize **size)
+{
+	const struct ov2659_framesize *fsize = &ov2659_framesizes[0];
+	const struct ov2659_framesize *match = NULL;
+	int i = ARRAY_SIZE(ov2659_framesizes);
+	unsigned int min_err = UINT_MAX;
+
+	while (i--) {
+		int err = abs(fsize->width - mf->width)
+				+ abs(fsize->height - mf->height);
+		if ((err < min_err) && (fsize->regs[0].addr)) {
+			min_err = err;
+			match = fsize;
+		}
+		fsize++;
+	}
+
+	if (!match)
+		match = &ov2659_framesizes[2];
+
+	mf->width  = match->width;
+	mf->height = match->height;
+
+	if (size)
+		*size = match;
+}
+
+static int ov2659_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			  struct v4l2_subdev_format *fmt)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	unsigned int index = ARRAY_SIZE(ov2659_formats);
+	struct v4l2_mbus_framefmt *mf = &fmt->format;
+	const struct ov2659_framesize *size = NULL;
+	struct ov2659 *ov2659 = to_ov2659(sd);
+
+	dev_dbg(&client->dev, "ov2659_set_fmt\n");
+
+	__ov2659_try_frame_size(mf, &size);
+
+	while (--index >= 0)
+		if (ov2659_formats[index].code == mf->code)
+			break;
+
+	if (index < 0)
+		return -EINVAL;
+
+	mf->colorspace	= V4L2_COLORSPACE_JPEG;
+	mf->code	= ov2659_formats[index].code;
+	mf->field	= V4L2_FIELD_NONE;
+
+	mutex_lock(&ov2659->lock);
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
+		if (fh) {
+			mf = v4l2_subdev_get_try_format(fh, fmt->pad);
+			*mf = fmt->format;
+		}
+	} else {
+		if (ov2659->streaming) {
+			mutex_unlock(&ov2659->lock);
+			return -EBUSY;
+		}
+
+		ov2659->frame_size = size;
+		ov2659->format = fmt->format;
+		ov2659->format_ctrl_regs =
+			ov2659_formats[index].format_ctrl_regs;
+	}
+
+	mutex_unlock(&ov2659->lock);
+	return 0;
+}
+
+static int ov2659_set_frame_size(struct ov2659 *ov2659)
+{
+	struct i2c_client *client = ov2659->client;
+
+	dev_dbg(&client->dev, "%s\n", __func__);
+
+	return ov2659_write_array(ov2659->client,
+				  ov2659->frame_size->regs);
+}
+
+static int ov2659_set_format(struct ov2659 *ov2659)
+{
+	struct i2c_client *client = ov2659->client;
+
+	dev_dbg(&client->dev, "%s\n", __func__);
+
+	return ov2659_write_array(ov2659->client,
+				  ov2659->format_ctrl_regs);
+}
+
+static int ov2659_s_stream(struct v4l2_subdev *sd, int on)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ov2659 *ov2659 = to_ov2659(sd);
+	int ret;
+
+	on = !!on;
+
+	dev_dbg(&client->dev, "%s: on: %d\n", __func__, on);
+
+	if (ov2659->streaming == on)
+		return 0;
+
+	mutex_lock(&ov2659->lock);
+
+	if (!on) {
+		/* Stop Streaming Sequence */
+		ov2659_set_streaming(ov2659, 0);
+		ov2659->streaming = on;
+		__ov2659_set_power(ov2659, 0);
+		mutex_unlock(&ov2659->lock);
+
+		return 0;
+	}
+
+	/* Start Streaming Sequence */
+	__ov2659_set_power(ov2659, 1);
+
+	ret = ov2659_write_array(client, ov2659_init_regs);
+	if (ret)
+		return ret;
+
+	ov2659_set_pixel_clock(ov2659);
+	ov2659_set_frame_size(ov2659);
+	ov2659_set_format(ov2659);
+
+	ov2659_set_streaming(ov2659, 1);
+
+	ov2659_reg_dump(client);
+	ov2659->streaming = on;
+	mutex_unlock(&ov2659->lock);
+
+	return 0;
+}
+
+/* -----------------------------------------------------------------------------
+ * V4L2 subdev internal operations
+ */
+
+static int ov2659_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(subdev);
+	struct v4l2_mbus_framefmt *format = v4l2_subdev_get_try_format(fh, 0);
+
+	dev_dbg(&client->dev, "%s:\n", __func__);
+
+	ov2659_get_default_format(format);
+
+	return ov2659_s_power(subdev, 1);
+}
+
+static int ov2659_close(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
+{
+	return ov2659_s_power(subdev, 0);
+}
+
+static const struct v4l2_subdev_core_ops ov2659_subdev_core_ops = {
+	.s_power = ov2659_s_power,
+	.log_status = v4l2_ctrl_subdev_log_status,
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_video_ops ov2659_subdev_video_ops = {
+	.s_stream = ov2659_s_stream,
+};
+
+static const struct v4l2_subdev_pad_ops ov2659_subdev_pad_ops = {
+	.enum_mbus_code = ov2659_enum_mbus_code,
+	.enum_frame_size = ov2659_enum_frame_sizes,
+	.get_fmt = ov2659_get_fmt,
+	.set_fmt = ov2659_set_fmt,
+};
+
+static const struct v4l2_subdev_ops ov2659_subdev_ops = {
+	.core  = &ov2659_subdev_core_ops,
+	.video = &ov2659_subdev_video_ops,
+	.pad   = &ov2659_subdev_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops ov2659_subdev_internal_ops = {
+	.open = ov2659_open,
+	.close = ov2659_close,
+};
+
+static int ov2659_detect_sensor(struct v4l2_subdev *sd)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct ov2659 *ov2659 = to_ov2659(sd);
+	u8 pid, ver;
+	int ret;
+
+	dev_dbg(&client->dev, "%s:\n", __func__);
+
+	mutex_lock(&ov2659->lock);
+	 __ov2659_set_power(ov2659, 1);
+	usleep_range(25000, 26000);
+
+	/* soft reset */
+	ret = ov2659_write(client, REG_SOFTWARE_RESET, 0x01);
+	if (ret != 0) {
+		dev_err(&client->dev, "Sensor soft reset failed\n");
+		ret = -ENODEV;
+	}
+	mdelay(5);
+
+	/* Check sensor revision */
+	ret = ov2659_read(client, REG_SC_CHIP_ID_H, &pid);
+	if (!ret)
+		ret = ov2659_read(client, REG_SC_CHIP_ID_L, &ver);
+
+	__ov2659_set_power(ov2659, 0);
+
+	if (!ret) {
+		ov2659->id = OV265X_ID(pid, ver);
+		if (ov2659->id != OV2659_ID) {
+			dev_err(&client->dev,
+				"Sensor detection failed (%04X, %d)\n",
+				ov2659->id, ret);
+			ret = -ENODEV;
+		} else {
+			dev_info(&client->dev, "Found OV%04X sensor\n",
+				ov2659->id);
+		}
+	}
+	mutex_unlock(&ov2659->lock);
+
+	return ret;
+}
+
+static struct ov2659_platform_data *
+ov2659_get_pdata(struct i2c_client *client)
+{
+	struct ov2659_platform_data *pdata;
+	struct device_node *endpoint;
+	int ret;
+
+	dev_dbg(&client->dev, "ov2659_get_pdata invoked\n");
+
+	if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
+		return client->dev.platform_data;
+
+	dev_dbg(&client->dev, "ov2659_get_pdata: DT Node found\n");
+
+	endpoint = of_graph_get_next_endpoint(client->dev.of_node, NULL);
+	if (!endpoint)
+		return NULL;
+
+	dev_dbg(&client->dev, "ov2659_get_pdata: endpoint found\n");
+
+	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		goto done;
+
+	ret = of_property_read_u32(endpoint, "clock-frequency",
+				   &pdata->mclk_frequency);
+	if (ret < 0) {
+		pdata->mclk_frequency = OV2659_DEFAULT_MCLK_FREQ;
+		dev_info(&client->dev, "using default %u Hz clock frequency\n",
+			 pdata->mclk_frequency);
+	}
+
+done:
+	of_node_put(endpoint);
+	return pdata;
+}
+
+static int ov2659_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	const struct ov2659_platform_data *pdata = ov2659_get_pdata(client);
+	struct v4l2_subdev *sd;
+	struct ov2659 *ov2659;
+	int ret;
+
+	dev_info(&client->dev, "ov2659_probe: debug: %d\n", debug);
+
+	if (!pdata) {
+		dev_err(&client->dev, "platform data not specified\n");
+		return -EINVAL;
+	}
+
+	ov2659 = devm_kzalloc(&client->dev, sizeof(*ov2659), GFP_KERNEL);
+	if (!ov2659)
+		return -ENOMEM;
+
+	ov2659->pdata = pdata;
+	mutex_init(&ov2659->lock);
+	ov2659->client = client;
+
+	sd = &ov2659->sd;
+	client->flags |= I2C_CLIENT_SCCB;
+	v4l2_i2c_subdev_init(sd, client, &ov2659_subdev_ops);
+	strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
+
+	sd->internal_ops = &ov2659_subdev_internal_ops;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+		     V4L2_SUBDEV_FL_HAS_EVENTS;
+
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	ov2659->pad.flags = MEDIA_PAD_FL_SOURCE;
+	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
+	ret = media_entity_init(&sd->entity, 1, &ov2659->pad, 0);
+	if (ret < 0)
+		return ret;
+#endif
+
+	ov2659_get_default_format(&ov2659->format);
+	ov2659->frame_size = &ov2659_framesizes[2];
+	ov2659->format_ctrl_regs = ov2659_formats[0].format_ctrl_regs;
+
+	ret = ov2659_detect_sensor(sd);
+	if (ret < 0)
+		goto error;
+
+	/* Calculate the PLL register value needed */
+	ov2659_pll_calc_params(ov2659);
+
+	ret = v4l2_async_register_subdev(&ov2659->sd);
+	if (ret)
+		goto error;
+
+	dev_info(&client->dev, "%s sensor driver registered !!\n", sd->name);
+
+	return 0;
+
+error:
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	media_entity_cleanup(&sd->entity);
+#endif
+	return ret;
+}
+
+static int ov2659_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+
+	v4l2_device_unregister_subdev(sd);
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	media_entity_cleanup(&sd->entity);
+#endif
+	return 0;
+}
+
+static const struct i2c_device_id ov2659_id[] = {
+	{ "ov2659", 0 },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i2c, ov2659_id);
+
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id ov2659_of_match[] = {
+	{ .compatible = "ovt,ov2659", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ov2659_of_match);
+#endif
+
+static struct i2c_driver ov2659_i2c_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name	= DRIVER_NAME,
+		.of_match_table = of_match_ptr(ov2659_of_match),
+	},
+	.probe		= ov2659_probe,
+	.remove		= ov2659_remove,
+	.id_table	= ov2659_id,
+};
+
+module_i2c_driver(ov2659_i2c_driver);
+
+MODULE_AUTHOR("Benoit Parrot <bparrot@ti.com>");
+MODULE_DESCRIPTION("OV2659 CMOS Image Sensor driver");
+MODULE_LICENSE("GPL");
diff --git a/include/media/ov2659.h b/include/media/ov2659.h
new file mode 100644
index 0000000..51cf277
--- /dev/null
+++ b/include/media/ov2659.h
@@ -0,0 +1,33 @@ 
+/*
+ * Omnivision OV2659 CMOS Image Sensor driver
+ *
+ * Copyright (C) 2015 Texas Instruments, Inc.
+ *
+ * Benoit Parrot <bparrot@ti.com>
+ * Lad, Prabhakar <prabhakar.csengg@gmail.com>
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef OV2659_H
+#define OV2659_H
+
+/**
+ * struct ov2659_platform_data - ov2659 driver platform data
+ * @mclk_frequency: the sensor's master clock frequency in Hz
+ */
+struct ov2659_platform_data {
+	unsigned int mclk_frequency;
+};
+#endif /* OV2659_H */