[PATCH v10 0/2] Add support for Omnivision OV5647

[PATCH v10 0/2] Add support for Omnivision OV5647

[Ramiro Oliveira]

Hello,

This patchset adds support for the Omnivision OV5647 sensor.

At the moment it only supports 640x480 in RAW 8.

This is the tenth version of the OV5647 camera driver patchset.

v10:
 - Add more error info to model check
 - Remove empty callback
Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
 - Change error message in read function
 - Change clock handling
 - Add error checking where it was missing
 - Remove debug messages
 - Remove the need for clock name
Suggested-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
 - Code refactoring

v9:
 - Remove unused struct
 - Remove comments
 - Refactor error handling in i2c r/w functions
 - Change declarations to single line.
 - Remove value assignment in variable declarion
 - Refactor configurion write loop 
 - Change the variable type that received ov5647_read() read value
 - Remove print from probe function
 - Remove unused device struct
 - Remove OF dependency from Kconfig
Suggested-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

v8:
 - Remove a part of the initialization procedure which wasn't doing 
 anything
 - Check for i2c read/writes return values
 - Add stream_on/off functions
Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>

v7:
 - Remove "0x" and leading 0 from DT documentation examples

v6:
 - Add example to DT documentation
 - Remove data-lanes and clock-lane property from DT
 - Add external clock property to DT
 - Order includes
 - Remove unused variables and functions
 - Add external clock handling
 - Add power on counter
 - Change from g/s_parm to g/s_frame_interval

v5:
 - Refactor code 
 - Change comments
 - Add missing error handling in some functions

v4: 
 - Add correct license
 - Revert debugging info to generic infrastructure
 - Turn defines into enums
 - Correct code style issues
 - Remove unused defines
 - Make sure all errors where being handled
 - Rename some functions to make code more readable
 - Add some debugging info

v3: 
 - No changes. Re-submitted due to lack of responses

v2: 
 - Corrections in DT documentation


Ramiro Oliveira (2):
  Documentation: DT: Add OV5647 bindings
  media: i2c: Add support for OV5647 sensor.

 .../devicetree/bindings/media/i2c/ov5647.txt       |  35 ++
 MAINTAINERS                                        |   7 +
 drivers/media/i2c/Kconfig                          |  11 +
 drivers/media/i2c/Makefile                         |   1 +
 drivers/media/i2c/ov5647.c                         | 636 +++++++++++++++++++++
 5 files changed, 690 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
 create mode 100644 drivers/media/i2c/ov5647.c

-- 
2.11.0

[PATCH v10 1/2] Documentation: DT: Add OV5647 bindings

[Ramiro Oliveira]

Create device tree bindings documentation.

Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>
---
 .../devicetree/bindings/media/i2c/ov5647.txt       | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5647.txt b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
new file mode 100644
index 000000000000..22e44945b661
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov5647.txt
@@ -0,0 +1,35 @@
+Omnivision OV5647 raw image sensor
+---------------------------------
+
+OV5647 is a raw image sensor with MIPI CSI-2 and CCP2 image data interfaces
+and CCI (I2C compatible) control bus.
+
+Required properties:
+
+- compatible		: "ovti,ov5647".
+- reg			: I2C slave address of the sensor.
+- clocks		: Reference to the xclk clock.
+
+The common video interfaces bindings (see video-interfaces.txt) should be
+used to specify link to the image data receiver. The OV5647 device
+node should contain one 'port' child node with an 'endpoint' subnode.
+
+Endpoint node mandatory properties:
+
+- remote-endpoint: A phandle to the bus receiver's endpoint node.
+
+Example:
+
+	i2c@2000 {
+		...
+		ov: camera@36 {
+			compatible = "ovti,ov5647";
+			reg = <0x36>;
+			clocks = <&camera_clk>;
+			port {
+				camera_1: endpoint {
+					remote-endpoint = <&csi1_ep1>;
+				};
+			};
+		};
+	};
-- 
2.11.0

[PATCH v10 2/2] media: i2c: Add support for OV5647 sensor.

[Ramiro Oliveira]

The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
and RAW 10 output formats, and MIPI CSI-2 interface.

The driver adds support for 640x480 RAW 8.

Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>
---
 MAINTAINERS                |   7 +
 drivers/media/i2c/Kconfig  |  11 +
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/ov5647.c | 636 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 655 insertions(+)
 create mode 100644 drivers/media/i2c/ov5647.c

diff --git a/MAINTAINERS b/MAINTAINERS
index b9e80b6bb745..630482636898 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9141,6 +9141,13 @@ M:	Harald Welte <laforge@gnumonks.org>
 S:	Maintained
 F:	drivers/char/pcmcia/cm4040_cs.*
 
+OMNIVISION OV5647 SENSOR DRIVER
+M:	Ramiro Oliveira <roliveir@synopsys.com>
+L:	linux-media@vger.kernel.org
+T:	git git://linuxtv.org/media_tree.git
+S:	Maintained
+F:	drivers/media/i2c/ov5647.c
+
 OMNIVISION OV7670 SENSOR DRIVER
 M:	Jonathan Corbet <corbet@lwn.net>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index cee1dae6e014..8435b99f38bc 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -531,6 +531,17 @@ config VIDEO_OV2659
 	  To compile this driver as a module, choose M here: the
 	  module will be called ov2659.
 
+config VIDEO_OV5647
+	tristate "OmniVision OV5647 sensor support"
+	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+	depends on MEDIA_CAMERA_SUPPORT
+	---help---
+	  This is a Video4Linux2 sensor-level driver for the OmniVision
+	  OV5647 camera.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ov5647.
+
 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 5bc7bbeb5499..ef2ccf65f94c 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -83,3 +83,4 @@ obj-$(CONFIG_VIDEO_IR_I2C)  += ir-kbd-i2c.o
 obj-$(CONFIG_VIDEO_ML86V7667)	+= ml86v7667.o
 obj-$(CONFIG_VIDEO_OV2659)	+= ov2659.o
 obj-$(CONFIG_VIDEO_TC358743)	+= tc358743.o
+obj-$(CONFIG_VIDEO_OV5647)	+= ov5647.o
diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
new file mode 100644
index 000000000000..07de127e84b2
--- /dev/null
+++ b/drivers/media/i2c/ov5647.c
@@ -0,0 +1,636 @@
+/*
+ * A V4L2 driver for OmniVision OV5647 cameras.
+ *
+ * Based on Samsung S5K6AAFX SXGA 1/6" 1.3M CMOS Image Sensor driver
+ * Copyright (C) 2011 Sylwester Nawrocki <s.nawrocki@samsung.com>
+ *
+ * Based on Omnivision OV7670 Camera Driver
+ * Copyright (C) 2006-7 Jonathan Corbet <corbet@lwn.net>
+ *
+ * Copyright (C) 2016, Synopsys, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed .as is. WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/videodev2.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-image-sizes.h>
+#include <media/v4l2-mediabus.h>
+#include <media/v4l2-of.h>
+
+#define SENSOR_NAME "ov5647"
+
+#define OV5647_SW_RESET		0x0103
+#define OV5647_REG_CHIPID_H	0x300A
+#define OV5647_REG_CHIPID_L	0x300B
+
+#define REG_TERM 0xfffe
+#define VAL_TERM 0xfe
+#define REG_DLY  0xffff
+
+#define OV5647_ROW_START		0x01
+#define OV5647_ROW_START_MIN		0
+#define OV5647_ROW_START_MAX		2004
+#define OV5647_ROW_START_DEF		54
+
+#define OV5647_COLUMN_START		0x02
+#define OV5647_COLUMN_START_MIN		0
+#define OV5647_COLUMN_START_MAX		2750
+#define OV5647_COLUMN_START_DEF		16
+
+#define OV5647_WINDOW_HEIGHT		0x03
+#define OV5647_WINDOW_HEIGHT_MIN	2
+#define OV5647_WINDOW_HEIGHT_MAX	2006
+#define OV5647_WINDOW_HEIGHT_DEF	1944
+
+#define OV5647_WINDOW_WIDTH		0x04
+#define OV5647_WINDOW_WIDTH_MIN		2
+#define OV5647_WINDOW_WIDTH_MAX		2752
+#define OV5647_WINDOW_WIDTH_DEF		2592
+
+struct regval_list {
+	u16 addr;
+	u8 data;
+};
+
+struct ov5647 {
+	struct v4l2_subdev		sd;
+	struct media_pad		pad;
+	struct mutex			lock;
+	struct v4l2_mbus_framefmt	format;
+	unsigned int			width;
+	unsigned int			height;
+	int				power_count;
+	struct clk			*xclk;
+};
+
+static inline struct ov5647 *to_state(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct ov5647, sd);
+}
+
+static struct regval_list sensor_oe_disable_regs[] = {
+	{0x3000, 0x00},
+	{0x3001, 0x00},
+	{0x3002, 0x00},
+};
+
+static struct regval_list sensor_oe_enable_regs[] = {
+	{0x3000, 0x0f},
+	{0x3001, 0xff},
+	{0x3002, 0xe4},
+};
+
+static struct regval_list ov5647_640x480[] = {
+	{0x0100, 0x00},
+	{0x0103, 0x01},
+	{0x3034, 0x08},
+	{0x3035, 0x21},
+	{0x3036, 0x46},
+	{0x303c, 0x11},
+	{0x3106, 0xf5},
+	{0x3821, 0x07},
+	{0x3820, 0x41},
+	{0x3827, 0xec},
+	{0x370c, 0x0f},
+	{0x3612, 0x59},
+	{0x3618, 0x00},
+	{0x5000, 0x06},
+	{0x5001, 0x01},
+	{0x5002, 0x41},
+	{0x5003, 0x08},
+	{0x5a00, 0x08},
+	{0x3000, 0x00},
+	{0x3001, 0x00},
+	{0x3002, 0x00},
+	{0x3016, 0x08},
+	{0x3017, 0xe0},
+	{0x3018, 0x44},
+	{0x301c, 0xf8},
+	{0x301d, 0xf0},
+	{0x3a18, 0x00},
+	{0x3a19, 0xf8},
+	{0x3c01, 0x80},
+	{0x3b07, 0x0c},
+	{0x380c, 0x07},
+	{0x380d, 0x68},
+	{0x380e, 0x03},
+	{0x380f, 0xd8},
+	{0x3814, 0x31},
+	{0x3815, 0x31},
+	{0x3708, 0x64},
+	{0x3709, 0x52},
+	{0x3808, 0x02},
+	{0x3809, 0x80},
+	{0x380a, 0x01},
+	{0x380b, 0xE0},
+	{0x3801, 0x00},
+	{0x3802, 0x00},
+	{0x3803, 0x00},
+	{0x3804, 0x0a},
+	{0x3805, 0x3f},
+	{0x3806, 0x07},
+	{0x3807, 0xa1},
+	{0x3811, 0x08},
+	{0x3813, 0x02},
+	{0x3630, 0x2e},
+	{0x3632, 0xe2},
+	{0x3633, 0x23},
+	{0x3634, 0x44},
+	{0x3636, 0x06},
+	{0x3620, 0x64},
+	{0x3621, 0xe0},
+	{0x3600, 0x37},
+	{0x3704, 0xa0},
+	{0x3703, 0x5a},
+	{0x3715, 0x78},
+	{0x3717, 0x01},
+	{0x3731, 0x02},
+	{0x370b, 0x60},
+	{0x3705, 0x1a},
+	{0x3f05, 0x02},
+	{0x3f06, 0x10},
+	{0x3f01, 0x0a},
+	{0x3a08, 0x01},
+	{0x3a09, 0x27},
+	{0x3a0a, 0x00},
+	{0x3a0b, 0xf6},
+	{0x3a0d, 0x04},
+	{0x3a0e, 0x03},
+	{0x3a0f, 0x58},
+	{0x3a10, 0x50},
+	{0x3a1b, 0x58},
+	{0x3a1e, 0x50},
+	{0x3a11, 0x60},
+	{0x3a1f, 0x28},
+	{0x4001, 0x02},
+	{0x4004, 0x02},
+	{0x4000, 0x09},
+	{0x4837, 0x24},
+	{0x4050, 0x6e},
+	{0x4051, 0x8f},
+	{0x0100, 0x01},
+};
+
+static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)
+{
+	int ret;
+	unsigned char data[3] = { reg >> 8, reg & 0xff, val};
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	ret = i2c_master_send(client, data, 3);
+	if (ret < 0)
+		dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
+				__func__, reg);
+
+	return ret;
+}
+
+static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
+{
+	int ret;
+	unsigned char data_w[2] = { reg >> 8, reg & 0xff };
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	ret = i2c_master_send(client, data_w, 2);
+	if (ret < 0) {
+		dev_dbg(&client->dev, "%s: i2c write error, reg: %x\n",
+			__func__, reg);
+		return ret;
+	}
+
+	ret = i2c_master_recv(client, val, 1);
+	if (ret < 0)
+		dev_dbg(&client->dev, "%s: i2c read error, reg: %x\n",
+				__func__, reg);
+
+	return ret;
+}
+
+static int ov5647_write_array(struct v4l2_subdev *sd,
+				struct regval_list *regs, int array_size)
+{
+	int i, ret;
+
+	for (i = 0; i < array_size; i++) {
+		ret = ov5647_write(sd, regs[i].addr, regs[i].data);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
+{
+	u8 channel_id;
+	int ret;
+
+	ret = ov5647_read(sd, 0x4814, &channel_id);
+	if (ret < 0)
+		return ret;
+
+	channel_id &= ~(3 << 6);
+	return ov5647_write(sd, 0x4814, channel_id | (channel << 6));
+}
+
+static int ov5647_stream_on(struct v4l2_subdev *sd)
+{
+	int ret;
+
+	ret = ov5647_write(sd, 0x4202, 0x00);
+	if (ret < 0)
+		return ret;
+
+	return ov5647_write(sd, 0x300D, 0x00);
+}
+
+static int ov5647_stream_off(struct v4l2_subdev *sd)
+{
+	int ret;
+
+	ret = ov5647_write(sd, 0x4202, 0x0f);
+	if (ret < 0)
+		return ret;
+
+	return ov5647_write(sd, 0x300D, 0x01);
+}
+
+static int set_sw_standby(struct v4l2_subdev *sd, bool standby)
+{
+	int ret;
+	u8 rdval;
+
+	ret = ov5647_read(sd, 0x0100, &rdval);
+	if (ret < 0)
+		return ret;
+
+	if (standby)
+		rdval &= ~0x01;
+	else
+		rdval |= 0x01;
+
+	return ov5647_write(sd, 0x0100, rdval);
+}
+
+static int __sensor_init(struct v4l2_subdev *sd)
+{
+	int ret;
+	u8 resetval, rdval;
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	dev_dbg(&client->dev, "sensor init\n");
+
+	ret = ov5647_read(sd, 0x0100, &rdval);
+	if (ret < 0)
+		return ret;
+
+	ret = ov5647_write_array(sd, ov5647_640x480,
+					ARRAY_SIZE(ov5647_640x480));
+	if (ret < 0) {
+		dev_err(&client->dev, "write sensor default regs error\n");
+		return ret;
+	}
+
+	ret = ov5647_set_virtual_channel(sd, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = ov5647_read(sd, 0x0100, &resetval);
+	if (ret < 0)
+		return ret;
+
+	if (!(resetval & 0x01)) {
+		dev_err(&client->dev, "Device was in SW standby");
+		ret = ov5647_write(sd, 0x0100, 0x01);
+		if (ret < 0)
+			return ret;
+	}
+
+	return ov5647_write(sd, 0x4800, 0x04);
+}
+
+static int ov5647_sensor_power(struct v4l2_subdev *sd, int on)
+{
+	int ret = 0;
+	struct ov5647 *ov5647 = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	mutex_lock(&ov5647->lock);
+
+	if (on && !ov5647->power_count)	{
+		dev_dbg(&client->dev, "OV5647 power on\n");
+
+		ret = clk_prepare_enable(ov5647->xclk);
+		if (ret < 0) {
+			dev_err(&client->dev, "clk prepare enable failed\n");
+			goto out;
+		}
+
+		ret = ov5647_write_array(sd, sensor_oe_enable_regs,
+				ARRAY_SIZE(sensor_oe_enable_regs));
+		if (ret < 0) {
+			clk_disable_unprepare(ov5647->xclk);
+			dev_err(&client->dev,
+				"write sensor_oe_enable_regs error\n");
+			goto out;
+		}
+
+		ret = __sensor_init(sd);
+		if (ret < 0) {
+			clk_disable_unprepare(ov5647->xclk);
+			dev_err(&client->dev,
+				"Camera not available, check Power\n");
+			goto out;
+		}
+	} else if (!on && ov5647->power_count == 1) {
+		dev_dbg(&client->dev, "OV5647 power off\n");
+
+		ret = ov5647_write_array(sd, sensor_oe_disable_regs,
+				ARRAY_SIZE(sensor_oe_disable_regs));
+
+		if (ret < 0)
+			dev_dbg(&client->dev, "disable oe failed\n");
+
+		ret = set_sw_standby(sd, true);
+
+		if (ret < 0)
+			dev_dbg(&client->dev, "soft stby failed\n");
+
+		clk_disable_unprepare(ov5647->xclk);
+	}
+
+	/* Update the power count. */
+	ov5647->power_count += on ? 1 : -1;
+	WARN_ON(ov5647->power_count < 0);
+
+out:
+	mutex_unlock(&ov5647->lock);
+
+	return ret;
+}
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+static int ov5647_sensor_get_register(struct v4l2_subdev *sd,
+				struct v4l2_dbg_register *reg)
+{
+	u8 val;
+	int ret;
+
+	ret = ov5647_read(sd, reg->reg & 0xff, &val);
+	if (ret < 0)
+		return ret;
+
+	reg->val = val;
+	reg->size = 1;
+
+	return 0;
+}
+
+static int ov5647_sensor_set_register(struct v4l2_subdev *sd,
+				const struct v4l2_dbg_register *reg)
+{
+	return ov5647_write(sd, reg->reg & 0xff, reg->val & 0xff);
+}
+#endif
+
+/**
+ * @short Subdev core operations registration
+ */
+static const struct v4l2_subdev_core_ops ov5647_subdev_core_ops = {
+	.s_power		= ov5647_sensor_power,
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	.g_register		= ov5647_sensor_get_register,
+	.s_register		= ov5647_sensor_set_register,
+#endif
+};
+
+static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	if (enable)
+		return ov5647_stream_on(sd);
+	else
+		return ov5647_stream_off(sd);
+}
+
+static const struct v4l2_subdev_video_ops ov5647_subdev_video_ops = {
+	.s_stream =		ov5647_s_stream,
+};
+
+static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
+				struct v4l2_subdev_pad_config *cfg,
+				struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->index > 0)
+		return -EINVAL;
+
+	code->code = MEDIA_BUS_FMT_SBGGR8_1X8;
+
+	return 0;
+}
+
+static const struct v4l2_subdev_pad_ops ov5647_subdev_pad_ops = {
+	.enum_mbus_code =	ov5647_enum_mbus_code,
+};
+
+static const struct v4l2_subdev_ops ov5647_subdev_ops = {
+	.core		= &ov5647_subdev_core_ops,
+	.video		= &ov5647_subdev_video_ops,
+	.pad		= &ov5647_subdev_pad_ops,
+};
+
+static int ov5647_detect(struct v4l2_subdev *sd)
+{
+	u8 read;
+	int ret;
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	ret = ov5647_write(sd, OV5647_SW_RESET, 0x01);
+	if (ret < 0)
+		return ret;
+
+	ret = ov5647_read(sd, OV5647_REG_CHIPID_H, &read);
+	if (ret < 0)
+		return ret;
+
+	if (read != 0x56) {
+		dev_err(&client->dev, "ID High expected 0x56 got %x", read);
+		return -ENODEV;
+	}
+
+	ret = ov5647_read(sd, OV5647_REG_CHIPID_L, &read);
+	if (ret < 0)
+		return ret;
+
+	if (read != 0x47) {
+		dev_err(&client->dev, "ID Low expected 0x47 got %x", read);
+		return -ENODEV;
+	}
+
+	return ov5647_write(sd, OV5647_SW_RESET, 0x00);
+}
+
+static int ov5647_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	struct v4l2_mbus_framefmt *format =
+				v4l2_subdev_get_try_format(sd, fh->pad, 0);
+	struct v4l2_rect *crop =
+				v4l2_subdev_get_try_crop(sd, fh->pad, 0);
+
+	crop->left = OV5647_COLUMN_START_DEF;
+	crop->top = OV5647_ROW_START_DEF;
+	crop->width = OV5647_WINDOW_WIDTH_DEF;
+	crop->height = OV5647_WINDOW_HEIGHT_DEF;
+
+	format->code = MEDIA_BUS_FMT_SBGGR8_1X8;
+
+	format->width = OV5647_WINDOW_WIDTH_DEF;
+	format->height = OV5647_WINDOW_HEIGHT_DEF;
+	format->field = V4L2_FIELD_NONE;
+	format->colorspace = V4L2_COLORSPACE_SRGB;
+
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops ov5647_subdev_internal_ops = {
+	.open = ov5647_open,
+};
+
+static int ov5647_parse_dt(struct device_node *np)
+{
+	struct v4l2_of_endpoint bus_cfg;
+	struct device_node *ep;
+
+	int ret;
+
+	ep = of_graph_get_next_endpoint(np, NULL);
+	if (!ep)
+		return -EINVAL;
+
+	ret = v4l2_of_parse_endpoint(ep, &bus_cfg);
+
+	of_node_put(ep);
+	return ret;
+}
+
+static int ov5647_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct ov5647 *sensor;
+	int ret;
+	struct v4l2_subdev *sd;
+	struct device_node *np = client->dev.of_node;
+	u32 xclk_freq;
+
+	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
+	if (!sensor)
+		return -ENOMEM;
+
+	if (IS_ENABLED(CONFIG_OF) && np) {
+		ret = ov5647_parse_dt(np);
+		if (ret) {
+			dev_err(dev, "DT parsing error: %d\n", ret);
+			return ret;
+		}
+	}
+
+	/* get system clock (xclk) */
+	sensor->xclk = devm_clk_get(dev, NULL);
+	if (IS_ERR(sensor->xclk)) {
+		dev_err(dev, "could not get xclk");
+		return PTR_ERR(sensor->xclk);
+	}
+
+	xclk_freq = clk_get_rate(sensor->xclk);
+	if (xclk_freq != 25000000) {
+		dev_err(dev, "Unsupported clock frequency: %u\n", xclk_freq);
+		return -EINVAL;
+	}
+
+	mutex_init(&sensor->lock);
+
+	sd = &sensor->sd;
+	v4l2_i2c_subdev_init(sd, client, &ov5647_subdev_ops);
+	sensor->sd.internal_ops = &ov5647_subdev_internal_ops;
+	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
+	sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
+	ret = media_entity_pads_init(&sd->entity, 1, &sensor->pad);
+	if (ret < 0)
+		goto mutex_remove;
+
+	ret = ov5647_detect(sd);
+	if (ret < 0)
+		goto error;
+
+	ret = v4l2_async_register_subdev(sd);
+	if (ret < 0)
+		goto error;
+
+	dev_dbg(dev, "OmniVision OV5647 camera driver probed\n");
+	return 0;
+error:
+	media_entity_cleanup(&sd->entity);
+mutex_remove:
+	mutex_destroy(&sensor->lock);
+	return ret;
+}
+
+static int ov5647_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct ov5647 *ov5647 = to_state(sd);
+
+	v4l2_async_unregister_subdev(&ov5647->sd);
+	media_entity_cleanup(&ov5647->sd.entity);
+	v4l2_device_unregister_subdev(sd);
+	mutex_destroy(&ov5647->lock);
+
+	return 0;
+}
+
+static const struct i2c_device_id ov5647_id[] = {
+	{ "ov5647", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ov5647_id);
+
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id ov5647_of_match[] = {
+	{ .compatible = "ovti,ov5647" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ov5647_of_match);
+#endif
+
+static struct i2c_driver ov5647_driver = {
+	.driver = {
+		.of_match_table = of_match_ptr(ov5647_of_match),
+		.name	= SENSOR_NAME,
+	},
+	.probe		= ov5647_probe,
+	.remove		= ov5647_remove,
+	.id_table	= ov5647_id,
+};
+
+module_i2c_driver(ov5647_driver);
+
+MODULE_AUTHOR("Ramiro Oliveira <roliveir@synopsys.com>");
+MODULE_DESCRIPTION("A low-level driver for OmniVision ov5647 sensors");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

Re: [PATCH v10 2/2] media: i2c: Add support for OV5647 sensor.

[Sakari Ailus]

Hi Ramiro,

On Mon, Mar 06, 2017 at 11:16:34AM +0000, Ramiro Oliveira wrote:
...
> +static int __sensor_init(struct v4l2_subdev *sd)
> +{
> +	int ret;
> +	u8 resetval, rdval;
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> +	dev_dbg(&client->dev, "sensor init\n");

This looks like a debugging time leftover. Please remove.

With that,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

...

> +static int ov5647_parse_dt(struct device_node *np)
> +{
> +	struct v4l2_of_endpoint bus_cfg;
> +	struct device_node *ep;
> +
> +	int ret;
> +
> +	ep = of_graph_get_next_endpoint(np, NULL);
> +	if (!ep)
> +		return -EINVAL;
> +
> +	ret = v4l2_of_parse_endpoint(ep, &bus_cfg);
> +
> +	of_node_put(ep);
> +	return ret;
> +}

This will conflict with my fwnode patchset. Let's see in which order the
patches will be merged, one of the sets has to be changed. The work is
trivial though.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v10 1/2] Documentation: DT: Add OV5647 bindings

[Rob Herring]

On Mon, Mar 06, 2017 at 11:16:33AM +0000, Ramiro Oliveira wrote:
> Create device tree bindings documentation.
> 
> Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>
> ---
>  .../devicetree/bindings/media/i2c/ov5647.txt       | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt

There's no changelog here, so I can't tell if anything is changed, but I 
acked v7. Please add acks when sending new versions.

Re: [PATCH v10 2/2] media: i2c: Add support for OV5647 sensor.

[Ramiro Oliveira]

Hi Sakari

On 3/7/2017 10:45 AM, Sakari Ailus wrote:
> Hi Ramiro,
> 
> On Mon, Mar 06, 2017 at 11:16:34AM +0000, Ramiro Oliveira wrote:
> ...
>> +static int __sensor_init(struct v4l2_subdev *sd)
>> +{
>> +	int ret;
>> +	u8 resetval, rdval;
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> +	dev_dbg(&client->dev, "sensor init\n");
> 
> This looks like a debugging time leftover. Please remove.
> 

Should I send a v11 with this change?

> With that,
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> ...
> 
>> +static int ov5647_parse_dt(struct device_node *np)
>> +{
>> +	struct v4l2_of_endpoint bus_cfg;
>> +	struct device_node *ep;
>> +
>> +	int ret;
>> +
>> +	ep = of_graph_get_next_endpoint(np, NULL);
>> +	if (!ep)
>> +		return -EINVAL;
>> +
>> +	ret = v4l2_of_parse_endpoint(ep, &bus_cfg);
>> +
>> +	of_node_put(ep);
>> +	return ret;
>> +}
> 
> This will conflict with my fwnode patchset. Let's see in which order the
> patches will be merged, one of the sets has to be changed. The work is
> trivial though.
> 

-- 
Best Regards

Ramiro Oliveira
Ramiro.Oliveira@synopsys.com

Re: [PATCH v10 1/2] Documentation: DT: Add OV5647 bindings

[Ramiro Oliveira]

Hi Rob

On 3/15/2017 4:42 PM, Rob Herring wrote:
> On Mon, Mar 06, 2017 at 11:16:33AM +0000, Ramiro Oliveira wrote:
>> Create device tree bindings documentation.
>>
>> Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>
>> ---
>>  .../devicetree/bindings/media/i2c/ov5647.txt       | 35 ++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
> 
> There's no changelog here, so I can't tell if anything is changed, but I 
> acked v7. Please add acks when sending new versions.
> 

The changelog is in the cover letter, although I didn't specify which changes
where made in the driver and which were made in the Documentation.

The only change was removing the clock name since there was only one clock used.

Should I keep your ack?

-- 
Best Regards

Ramiro Oliveira
Ramiro.Oliveira@synopsys.com

Re: [PATCH v10 1/2] Documentation: DT: Add OV5647 bindings

[Rob Herring]

On Wed, Mar 15, 2017 at 11:51 AM, Ramiro Oliveira
<Ramiro.Oliveira@synopsys.com> wrote:
> Hi Rob
>
> On 3/15/2017 4:42 PM, Rob Herring wrote:
>> On Mon, Mar 06, 2017 at 11:16:33AM +0000, Ramiro Oliveira wrote:
>>> Create device tree bindings documentation.
>>>
>>> Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>
>>> ---
>>>  .../devicetree/bindings/media/i2c/ov5647.txt       | 35 ++++++++++++++++++++++
>>>  1 file changed, 35 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5647.txt
>>
>> There's no changelog here, so I can't tell if anything is changed, but I
>> acked v7. Please add acks when sending new versions.
>>
>
> The changelog is in the cover letter, although I didn't specify which changes
> where made in the driver and which were made in the Documentation.
>
> The only change was removing the clock name since there was only one clock used.
>
> Should I keep your ack?

Yes.

Rob

Re: [PATCH v10 2/2] media: i2c: Add support for OV5647 sensor.

[Sakari Ailus]

Hi Ramiro,

On Wed, Mar 15, 2017 at 04:45:16PM +0000, Ramiro Oliveira wrote:
> Hi Sakari
> 
> On 3/7/2017 10:45 AM, Sakari Ailus wrote:
> > Hi Ramiro,
> > 
> > On Mon, Mar 06, 2017 at 11:16:34AM +0000, Ramiro Oliveira wrote:
> > ...
> >> +static int __sensor_init(struct v4l2_subdev *sd)
> >> +{
> >> +	int ret;
> >> +	u8 resetval, rdval;
> >> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> >> +
> >> +	dev_dbg(&client->dev, "sensor init\n");
> > 
> > This looks like a debugging time leftover. Please remove.
> > 
> 
> Should I send a v11 with this change?

Please do; you can add my ack on that one.

> 
> > With that,
> > 
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > ...
> > 

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v10 1/2] Documentation: DT: Add OV5647 bindings

[Vladimir Zapolskiy]

Hi Ramiro,

On 03/06/2017 01:16 PM, Ramiro Oliveira wrote:
> Create device tree bindings documentation.
> 
> Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>

The device tree binding description looks perfect from my perspective.

Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

--
With best wishes,
Vladimir

Re: [PATCH v10 2/2] media: i2c: Add support for OV5647 sensor.

[Vladimir Zapolskiy]

Hi Ramiro,

On 03/06/2017 01:16 PM, Ramiro Oliveira wrote:
> The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
> and RAW 10 output formats, and MIPI CSI-2 interface.
> 
> The driver adds support for 640x480 RAW 8.
> 
> Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>

All updates are fine, thank you. Feel free to add my

Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

> ---
>  MAINTAINERS                |   7 +
>  drivers/media/i2c/Kconfig  |  11 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/ov5647.c | 636 +++++++++++++++++++++++++++++++++++++++++++++

I see this version has 100 LoC less in comparison to v8, good result.

[snip]

> +
> +static const struct v4l2_subdev_pad_ops ov5647_subdev_pad_ops = {
> +	.enum_mbus_code =	ov5647_enum_mbus_code,

Nitpicking, above it's better to swap tab and space symbols around '='.

> +};
> +
> +static const struct v4l2_subdev_ops ov5647_subdev_ops = {
> +	.core		= &ov5647_subdev_core_ops,
> +	.video		= &ov5647_subdev_video_ops,
> +	.pad		= &ov5647_subdev_pad_ops,
> +};
> +

--
With best wishes,
Vladimir