[REVIEW PATCH 0/6] exynos4-is: Asynchronous subdev registration support

[REVIEW PATCH 0/6] exynos4-is: Asynchronous subdev registration support

[Sylwester Nawrocki]

This patch series is a refactoring of the exynos4-is driver to get rid
of the common fimc-is-sensor driver and to adapt it to use "standard"
sensor subdev drivers, one per each image sensor type.
Then a clock provider is added to the exynos4-is driver and the s5k6a3
subdev is modified to use one of the clocks registered by exynos4-is.

Arun, I think you could reuse the s5k6a3 sensor for your work on the
Exynos5 FIMC-IS driver. One advantage of separate sensor drivers is
that the power on/off sequences can be written specifically for each
sensor. We are probably going to need such sequences per board in
future. Also having the clock control inside the sensor subdev allows
to better match the hardware power on/off sequence requirements,
however the S5K6A3 sensor can have active clock signal on its clock
input pin even when all its power supplies are turned off.

I'm posting this series before having a proper implementation for
clk_unregister() in the clock framework, so you are not blocked with
your Exynos5 FIMC-IS works.

This series with all dependencies can be found at:
http://git.linuxtv.org/snawrocki/samsung.git/exynos4-is-clk

Thanks,
Sylwester


Sylwester Nawrocki (6):
  V4L: Add driver for s5k6a3 image sensor
  V4L: s5k6a3: Add support for asynchronous subdev registration
  exynos4-is: Simplify sclk_cam clocks handling
  exynos4-is: Add clock provider for the external clocks
  exynos4-is: Use external s5k6a3 sensor driver
  exynos4-is: Add support for asynchronous sensor subddevs registration

 .../devicetree/bindings/media/samsung-fimc.txt     |   17 +-
 drivers/media/i2c/Kconfig                          |    8 +
 drivers/media/i2c/Makefile                         |    1 +
 drivers/media/i2c/s5k6a3.c                         |  356 ++++++++++++++++++++
 drivers/media/platform/exynos4-is/fimc-is-regs.c   |    2 +-
 drivers/media/platform/exynos4-is/fimc-is-sensor.c |  285 +---------------
 drivers/media/platform/exynos4-is/fimc-is-sensor.h |   49 +--
 drivers/media/platform/exynos4-is/fimc-is.c        |   96 +++---
 drivers/media/platform/exynos4-is/fimc-is.h        |    4 +-
 drivers/media/platform/exynos4-is/media-dev.c      |  268 ++++++++++-----
 drivers/media/platform/exynos4-is/media-dev.h      |   31 +-
 11 files changed, 650 insertions(+), 467 deletions(-)
 create mode 100644 drivers/media/i2c/s5k6a3.c

--
1.7.9.5

[REVIEW PATCH 1/6] V4L: Add driver for s5k6a3 image sensor

[Sylwester Nawrocki]

This patch adds subdev driver for Samsung S5K6A3 raw image sensor.
As it is intended at the moment to be used only with the Exynos
FIMC-IS (camera ISP) subsystem it is pretty minimal subdev driver.
It doesn't do any I2C communication since the sensor is controlled
by the ISP and its own firmware.
This driver can be updated in future, should anyone need it to be
a regular subdev driver where the main CPU communicates with the
sensor directly.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/i2c/Kconfig  |    8 ++
 drivers/media/i2c/Makefile |    1 +
 drivers/media/i2c/s5k6a3.c |  315 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 324 insertions(+)
 create mode 100644 drivers/media/i2c/s5k6a3.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 3367ec2..dbd9cbb 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -564,6 +564,14 @@ config VIDEO_S5K6AA
 	  This is a V4L2 sensor-level driver for Samsung S5K6AA(FX) 1.3M
 	  camera sensor with an embedded SoC image signal processor.
 
+config VIDEO_S5K6A3
+	tristate "Samsung S5K6A3 sensor support"
+	depends on MEDIA_CAMERA_SUPPORT
+	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF
+	---help---
+	  This is a V4L2 sensor-level driver for Samsung S5K6A3 raw
+	  camera sensor.
+
 config VIDEO_S5K4ECGX
         tristate "Samsung S5K4ECGX sensor support"
         depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index d590925..44998a2 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -64,6 +64,7 @@ obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o
 obj-$(CONFIG_VIDEO_SR030PC30)	+= sr030pc30.o
 obj-$(CONFIG_VIDEO_NOON010PC30)	+= noon010pc30.o
 obj-$(CONFIG_VIDEO_S5K6AA)	+= s5k6aa.o
+obj-$(CONFIG_VIDEO_S5K6A3)	+= s5k6a3.o
 obj-$(CONFIG_VIDEO_S5K4ECGX)	+= s5k4ecgx.o
 obj-$(CONFIG_VIDEO_S5K5BAF)	+= s5k5baf.o
 obj-$(CONFIG_VIDEO_S5C73M3)	+= s5c73m3/
diff --git a/drivers/media/i2c/s5k6a3.c b/drivers/media/i2c/s5k6a3.c
new file mode 100644
index 0000000..21680fa
--- /dev/null
+++ b/drivers/media/i2c/s5k6a3.c
@@ -0,0 +1,315 @@
+/*
+ * Samsung S5K6A3 image sensor driver
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/videodev2.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-subdev.h>
+
+#define S5K6A3_SENSOR_MAX_WIDTH		1392
+#define S5K6A3_SENSOR_MAX_HEIGHT	1392
+#define S5K6A3_SENSOR_MIN_WIDTH		32
+#define S5K6A3_SENSOR_MIN_HEIGHT	32
+
+#define S5K6A3_DEF_PIX_WIDTH		1296
+#define S5K6A3_DEF_PIX_HEIGHT		732
+
+#define S5K6A3_DRV_NAME			"S5K6A3"
+
+#define S5K6A3_NUM_SUPPLIES		2
+
+/**
+ * struct s5k6a3 - fimc-is sensor data structure
+ * @dev: pointer to this I2C client device structure
+ * @subdev: the image sensor's v4l2 subdev
+ * @pad: subdev media source pad
+ * @supplies: image sensor's voltage regulator supplies
+ * @gpio_reset: GPIO connected to the sensor's reset pin
+ * @lock: mutex protecting the structure's members below
+ * @format: media bus format at the sensor's source pad
+ */
+struct s5k6a3 {
+	struct device *dev;
+	struct v4l2_subdev subdev;
+	struct media_pad pad;
+	struct regulator_bulk_data supplies[S5K6A3_NUM_SUPPLIES];
+	int gpio_reset;
+	struct mutex lock;
+	struct v4l2_mbus_framefmt format;
+	u32 clock_frequency;
+};
+
+static const char * const s5k6a3_supply_names[] = {
+	"svdda",
+	"svddio"
+};
+
+static inline struct s5k6a3 *sd_to_s5k6a3(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct s5k6a3, subdev);
+}
+
+static const struct v4l2_mbus_framefmt s5k6a3_formats[] = {
+	{
+		.code = V4L2_MBUS_FMT_SGRBG10_1X10,
+		.colorspace = V4L2_COLORSPACE_SRGB,
+		.field = V4L2_FIELD_NONE,
+	}
+};
+
+static const struct v4l2_mbus_framefmt *find_sensor_format(
+	struct v4l2_mbus_framefmt *mf)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(s5k6a3_formats); i++)
+		if (mf->code == s5k6a3_formats[i].code)
+			return &s5k6a3_formats[i];
+
+	return &s5k6a3_formats[0];
+}
+
+static int s5k6a3_enum_mbus_code(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_fh *fh,
+				  struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->index >= ARRAY_SIZE(s5k6a3_formats))
+		return -EINVAL;
+
+	code->code = s5k6a3_formats[code->index].code;
+	return 0;
+}
+
+static void s5k6a3_try_format(struct v4l2_mbus_framefmt *mf)
+{
+	const struct v4l2_mbus_framefmt *fmt;
+
+	fmt = find_sensor_format(mf);
+	mf->code = fmt->code;
+	v4l_bound_align_image(&mf->width, S5K6A3_SENSOR_MIN_WIDTH,
+			      S5K6A3_SENSOR_MAX_WIDTH, 0,
+			      &mf->height, S5K6A3_SENSOR_MIN_HEIGHT,
+			      S5K6A3_SENSOR_MAX_HEIGHT, 0, 0);
+}
+
+static struct v4l2_mbus_framefmt *__s5k6a3_get_format(
+		struct s5k6a3 *sensor, struct v4l2_subdev_fh *fh,
+		u32 pad, enum v4l2_subdev_format_whence which)
+{
+	if (which == V4L2_SUBDEV_FORMAT_TRY)
+		return fh ? v4l2_subdev_get_try_format(fh, pad) : NULL;
+
+	return &sensor->format;
+}
+
+static int s5k6a3_set_fmt(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_fh *fh,
+				  struct v4l2_subdev_format *fmt)
+{
+	struct s5k6a3 *sensor = sd_to_s5k6a3(sd);
+	struct v4l2_mbus_framefmt *mf;
+
+	s5k6a3_try_format(&fmt->format);
+
+	mf = __s5k6a3_get_format(sensor, fh, fmt->pad, fmt->which);
+	if (mf) {
+		mutex_lock(&sensor->lock);
+		if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+			*mf = fmt->format;
+		mutex_unlock(&sensor->lock);
+	}
+	return 0;
+}
+
+static int s5k6a3_get_fmt(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_fh *fh,
+				  struct v4l2_subdev_format *fmt)
+{
+	struct s5k6a3 *sensor = sd_to_s5k6a3(sd);
+	struct v4l2_mbus_framefmt *mf;
+
+	mf = __s5k6a3_get_format(sensor, fh, fmt->pad, fmt->which);
+
+	mutex_lock(&sensor->lock);
+	fmt->format = *mf;
+	mutex_unlock(&sensor->lock);
+	return 0;
+}
+
+static struct v4l2_subdev_pad_ops s5k6a3_pad_ops = {
+	.enum_mbus_code	= s5k6a3_enum_mbus_code,
+	.get_fmt	= s5k6a3_get_fmt,
+	.set_fmt	= s5k6a3_set_fmt,
+};
+
+static int s5k6a3_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	struct v4l2_mbus_framefmt *format = v4l2_subdev_get_try_format(fh, 0);
+
+	*format		= s5k6a3_formats[0];
+	format->width	= S5K6A3_DEF_PIX_WIDTH;
+	format->height	= S5K6A3_DEF_PIX_HEIGHT;
+
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops s5k6a3_sd_internal_ops = {
+	.open = s5k6a3_open,
+};
+
+static int s5k6a3_s_power(struct v4l2_subdev *sd, int on)
+{
+	struct s5k6a3 *sensor = sd_to_s5k6a3(sd);
+	int gpio = sensor->gpio_reset;
+	int ret;
+
+	if (on) {
+		ret = pm_runtime_get(sensor->dev);
+		if (ret < 0)
+			return ret;
+
+		ret = regulator_bulk_enable(S5K6A3_NUM_SUPPLIES,
+					    sensor->supplies);
+		if (ret < 0) {
+			pm_runtime_put(sensor->dev);
+			return ret;
+		}
+
+		if (gpio_is_valid(gpio)) {
+			gpio_set_value(gpio, 1);
+			usleep_range(600, 800);
+			gpio_set_value(gpio, 0);
+			usleep_range(10000, 11000);
+			gpio_set_value(gpio, 1);
+		}
+
+		/* Delay needed for the sensor initialization */
+		msleep(20);
+	} else {
+		if (gpio_is_valid(gpio))
+			gpio_set_value(gpio, 0);
+
+		ret = regulator_bulk_disable(S5K6A3_NUM_SUPPLIES,
+					     sensor->supplies);
+		if (!ret)
+			pm_runtime_put(sensor->dev);
+	}
+	return ret;
+}
+
+static struct v4l2_subdev_core_ops s5k6a3_core_ops = {
+	.s_power = s5k6a3_s_power,
+};
+
+static struct v4l2_subdev_ops s5k6a3_subdev_ops = {
+	.core = &s5k6a3_core_ops,
+	.pad = &s5k6a3_pad_ops,
+};
+
+static int s5k6a3_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct s5k6a3 *sensor;
+	struct v4l2_subdev *sd;
+	int gpio, i, ret;
+
+	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
+	if (!sensor)
+		return -ENOMEM;
+
+	mutex_init(&sensor->lock);
+	sensor->gpio_reset = -EINVAL;
+	sensor->dev = dev;
+
+	gpio = of_get_gpio_flags(dev->of_node, 0, NULL);
+	if (gpio_is_valid(gpio)) {
+		ret = devm_gpio_request_one(dev, gpio, GPIOF_OUT_INIT_LOW,
+							S5K6A3_DRV_NAME);
+		if (ret < 0)
+			return ret;
+	}
+	sensor->gpio_reset = gpio;
+
+	for (i = 0; i < S5K6A3_NUM_SUPPLIES; i++)
+		sensor->supplies[i].supply = s5k6a3_supply_names[i];
+
+	ret = devm_regulator_bulk_get(&client->dev, S5K6A3_NUM_SUPPLIES,
+				      sensor->supplies);
+	if (ret < 0)
+		return ret;
+
+	sd = &sensor->subdev;
+	v4l2_i2c_subdev_init(sd, client, &s5k6a3_subdev_ops);
+	snprintf(sd->name, sizeof(sd->name), S5K6A3_DRV_NAME);
+	sensor->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+	sensor->format.code = s5k6a3_formats[0].code;
+	sensor->format.width = S5K6A3_DEF_PIX_WIDTH;
+	sensor->format.height = S5K6A3_DEF_PIX_HEIGHT;
+
+	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
+	ret = media_entity_init(&sd->entity, 1, &sensor->pad, 0);
+	if (ret < 0)
+		return ret;
+
+	pm_runtime_no_callbacks(dev);
+	pm_runtime_enable(dev);
+
+	return 0;
+}
+
+static int s5k6a3_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	media_entity_cleanup(&sd->entity);
+	return 0;
+}
+
+static const struct i2c_device_id s5k6a3_ids[] = {
+	{ }
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id s5k6a3_of_match[] = {
+	{ .compatible = "samsung,s5k6a3" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, s5k6a3_of_match);
+#endif
+
+static struct i2c_driver s5k6a3_driver = {
+	.driver = {
+		.of_match_table	= of_match_ptr(s5k6a3_of_match),
+		.name		= S5K6A3_DRV_NAME,
+		.owner		= THIS_MODULE,
+	},
+	.probe		= s5k6a3_probe,
+	.remove		= s5k6a3_remove,
+	.id_table	= s5k6a3_ids,
+};
+
+module_i2c_driver(s5k6a3_driver);
+
+MODULE_DESCRIPTION("S5K6A3 image sensor subdev driver");
+MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

[REVIEW PATCH 2/6] V4L: s5k6a3: Add support for asynchronous subdev registration

[Sylwester Nawrocki]

This patch converts the driver to use v4l2 asynchronous subdev
registration API an the common clock API.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/i2c/s5k6a3.c |   63 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/s5k6a3.c b/drivers/media/i2c/s5k6a3.c
index 21680fa..ccbb4fc 100644
--- a/drivers/media/i2c/s5k6a3.c
+++ b/drivers/media/i2c/s5k6a3.c
@@ -34,6 +34,7 @@
 #define S5K6A3_DEF_PIX_HEIGHT		732
 
 #define S5K6A3_DRV_NAME			"S5K6A3"
+#define S5K6A3_CLK_NAME			"mclk"
 
 #define S5K6A3_NUM_SUPPLIES		2
 
@@ -55,6 +56,7 @@ struct s5k6a3 {
 	int gpio_reset;
 	struct mutex lock;
 	struct v4l2_mbus_framefmt format;
+	struct clk *clock;
 	u32 clock_frequency;
 };
 
@@ -180,19 +182,29 @@ static int s5k6a3_s_power(struct v4l2_subdev *sd, int on)
 {
 	struct s5k6a3 *sensor = sd_to_s5k6a3(sd);
 	int gpio = sensor->gpio_reset;
-	int ret;
+	int ret = 0;
 
 	if (on) {
+		sensor->clock = clk_get(sensor->dev, S5K6A3_CLK_NAME);
+		if (IS_ERR(sensor->clock))
+			return PTR_ERR(sensor->clock);
+
+		ret = clk_set_rate(sensor->clock, sensor->clock_frequency);
+		if (ret < 0)
+			goto clk_put;
+
 		ret = pm_runtime_get(sensor->dev);
 		if (ret < 0)
-			return ret;
+			goto clk_put;
 
 		ret = regulator_bulk_enable(S5K6A3_NUM_SUPPLIES,
 					    sensor->supplies);
-		if (ret < 0) {
-			pm_runtime_put(sensor->dev);
-			return ret;
-		}
+		if (ret < 0)
+			goto rpm_put;
+
+		ret = clk_prepare_enable(sensor->clock);
+		if (ret < 0)
+			goto reg_dis;
 
 		if (gpio_is_valid(gpio)) {
 			gpio_set_value(gpio, 1);
@@ -208,10 +220,14 @@ static int s5k6a3_s_power(struct v4l2_subdev *sd, int on)
 		if (gpio_is_valid(gpio))
 			gpio_set_value(gpio, 0);
 
-		ret = regulator_bulk_disable(S5K6A3_NUM_SUPPLIES,
-					     sensor->supplies);
-		if (!ret)
-			pm_runtime_put(sensor->dev);
+		clk_disable_unprepare(sensor->clock);
+reg_dis:
+		regulator_bulk_disable(S5K6A3_NUM_SUPPLIES,
+						sensor->supplies);
+rpm_put:
+		pm_runtime_put(sensor->dev);
+clk_put:
+		clk_put(sensor->clock);
 	}
 	return ret;
 }
@@ -239,6 +255,7 @@ static int s5k6a3_probe(struct i2c_client *client,
 
 	mutex_init(&sensor->lock);
 	sensor->gpio_reset = -EINVAL;
+	sensor->clock = ERR_PTR(-EINVAL);
 	sensor->dev = dev;
 
 	gpio = of_get_gpio_flags(dev->of_node, 0, NULL);
@@ -250,6 +267,13 @@ static int s5k6a3_probe(struct i2c_client *client,
 	}
 	sensor->gpio_reset = gpio;
 
+	if (of_property_read_u32(dev->of_node, "clock-frequency",
+				 &sensor->clock_frequency)) {
+		dev_err(dev, "clock-frequency property not found at %s\n",
+						dev->of_node->full_name);
+		return -EINVAL;
+	}
+
 	for (i = 0; i < S5K6A3_NUM_SUPPLIES; i++)
 		sensor->supplies[i].supply = s5k6a3_supply_names[i];
 
@@ -258,6 +282,11 @@ static int s5k6a3_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
+	/* Defer probing if the clock is not available yet */
+	sensor->clock = clk_get(dev, S5K6A3_CLK_NAME);
+	if (IS_ERR(sensor->clock))
+		return -EPROBE_DEFER;
+
 	sd = &sensor->subdev;
 	v4l2_i2c_subdev_init(sd, client, &s5k6a3_subdev_ops);
 	snprintf(sd->name, sizeof(sd->name), S5K6A3_DRV_NAME);
@@ -275,12 +304,24 @@ static int s5k6a3_probe(struct i2c_client *client,
 	pm_runtime_no_callbacks(dev);
 	pm_runtime_enable(dev);
 
-	return 0;
+	ret = v4l2_async_register_subdev(sd);
+
+	/*
+	 * Don't hold reference to the clock to avoid circular dependency
+	 * between the subdev and the host driver, in case the host is
+	 * a supplier of the clock.
+	 * clk_get()/clk_put() will be called in s_power callback.
+	 */
+	clk_put(sensor->clock);
+
+	return ret;
 }
 
 static int s5k6a3_remove(struct i2c_client *client)
 {
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+
+	v4l2_async_unregister_subdev(sd);
 	media_entity_cleanup(&sd->entity);
 	return 0;
 }
-- 
1.7.9.5

[REVIEW PATCH 3/6] exynos4-is: Simplify sclk_cam clocks handling

[Sylwester Nawrocki]

Use clk_prepare_enable()/clk_disable_unprepare() instead of
separately prearing/unparing the clk_cam clocks. This simplifies
the code that is now mostly not going to be used, function
__fimc_md_set_camclk() is only left for S5PV210 platform which
is not yet converted to Device Tree.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/exynos4-is/media-dev.c |   13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index 91f21e2..41366fe 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -1150,7 +1150,6 @@ static void fimc_md_put_clocks(struct fimc_md *fmd)
 	while (--i >= 0) {
 		if (IS_ERR(fmd->camclk[i].clock))
 			continue;
-		clk_unprepare(fmd->camclk[i].clock);
 		clk_put(fmd->camclk[i].clock);
 		fmd->camclk[i].clock = ERR_PTR(-EINVAL);
 	}
@@ -1169,7 +1168,7 @@ static int fimc_md_get_clocks(struct fimc_md *fmd)
 	struct device *dev = NULL;
 	char clk_name[32];
 	struct clk *clock;
-	int ret, i;
+	int i, ret = 0;
 
 	for (i = 0; i < FIMC_MAX_CAMCLKS; i++)
 		fmd->camclk[i].clock = ERR_PTR(-EINVAL);
@@ -1187,12 +1186,6 @@ static int fimc_md_get_clocks(struct fimc_md *fmd)
 			ret = PTR_ERR(clock);
 			break;
 		}
-		ret = clk_prepare(clock);
-		if (ret < 0) {
-			clk_put(clock);
-			fmd->camclk[i].clock = ERR_PTR(-EINVAL);
-			break;
-		}
 		fmd->camclk[i].clock = clock;
 	}
 	if (ret)
@@ -1249,7 +1242,7 @@ static int __fimc_md_set_camclk(struct fimc_md *fmd,
 			ret = pm_runtime_get_sync(fmd->pmf);
 			if (ret < 0)
 				return ret;
-			ret = clk_enable(camclk->clock);
+			ret = clk_prepare_enable(camclk->clock);
 			dbg("Enabled camclk %d: f: %lu", si->clk_id,
 			    clk_get_rate(camclk->clock));
 		}
@@ -1260,7 +1253,7 @@ static int __fimc_md_set_camclk(struct fimc_md *fmd,
 		return 0;
 
 	if (--camclk->use_count == 0) {
-		clk_disable(camclk->clock);
+		clk_disable_unprepare(camclk->clock);
 		pm_runtime_put(fmd->pmf);
 		dbg("Disabled camclk %d", si->clk_id);
 	}
-- 
1.7.9.5

[REVIEW PATCH 4/6] exynos4-is: Add clock provider for the external clocks

[Sylwester Nawrocki]

This patch adds clock provider to expose the sclk_cam0/1 clocks
for image sensor subdevs.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../devicetree/bindings/media/samsung-fimc.txt     |   17 +++-
 drivers/media/platform/exynos4-is/media-dev.c      |   92 ++++++++++++++++++++
 drivers/media/platform/exynos4-is/media-dev.h      |   19 +++-
 3 files changed, 125 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt
index 96312f6..04a2b87 100644
--- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
@@ -91,6 +91,15 @@ Optional properties
 - samsung,camclk-out : specifies clock output for remote sensor,
 		       0 - CAM_A_CLKOUT, 1 - CAM_B_CLKOUT;
 
+'clock-controller' node (optional)
+----------------------------------
+
+The purpose of this node is to define a clock provider for external image
+sensors and link any of the CAM_?_CLKOUT clock outputs with related external
+clock consumer device. Properties specific to this node are described in
+../clock/clock-bindings.txt.
+
+
 Image sensor nodes
 ------------------
 
@@ -114,7 +123,7 @@ Example:
 			vddio-supply = <...>;
 
 			clock-frequency = <24000000>;
-			clocks = <...>;
+			clocks = <&camclk 1>;
 			clock-names = "mclk";
 
 			port {
@@ -135,7 +144,7 @@ Example:
 			vddio-supply = <...>;
 
 			clock-frequency = <24000000>;
-			clocks = <...>;
+			clocks = <&camclk 0>;
 			clock-names = "mclk";
 
 			port {
@@ -156,6 +165,10 @@ Example:
 		pinctrl-names = "default";
 		pinctrl-0 = <&cam_port_a_clk_active>;
 
+		camclk: clock-controller {
+		       #clock-cells = 1;
+		};
+
 		/* parallel camera ports */
 		parallel-ports {
 			/* camera A input */
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index 41366fe..346e1e0 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -11,6 +11,8 @@
  */
 
 #include <linux/bug.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
@@ -1438,6 +1440,86 @@ static int fimc_md_get_pinctrl(struct fimc_md *fmd)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+struct cam_clk {
+	struct clk_hw hw;
+	struct fimc_md *fmd;
+};
+#define to_cam_clk(_hw) container_of(_hw, struct cam_clk, hw)
+
+static int cam_clk_prepare(struct clk_hw *hw)
+{
+	struct cam_clk *camclk = to_cam_clk(hw);
+	int ret = pm_runtime_get_sync(camclk->fmd->pmf);
+
+	return ret < 0 ? ret : 0;
+}
+
+static void cam_clk_unprepare(struct clk_hw *hw)
+{
+	struct cam_clk *camclk = to_cam_clk(hw);
+	pm_runtime_put_sync(camclk->fmd->pmf);
+}
+
+static const struct clk_ops cam_clk_ops = {
+	.prepare = cam_clk_prepare,
+	.unprepare = cam_clk_unprepare,
+};
+
+static const char *cam_clk_p_names[] = { "sclk_cam0", "sclk_cam1" };
+
+static int fimc_md_register_clk_provider(struct fimc_md *fmd)
+{
+	struct cam_clk_provider *clk_provider = &fmd->clk_provider;
+	struct device *dev = &fmd->pdev->dev;
+	struct device_node *node;
+	unsigned int nclocks;
+
+	node = of_get_child_by_name(dev->of_node, "clock-controller");
+	if (!node) {
+		dev_warn(dev, "clock-controller node at %s not found\n",
+					dev->of_node->full_name);
+		return 0;
+	}
+	/* Instantiate the clocks */
+	for (nclocks = 0; nclocks < FIMC_MAX_CAMCLKS; nclocks++) {
+		struct clk_init_data init;
+		char clk_name[16];
+		struct clk *clk;
+		struct cam_clk *camclk;
+
+		camclk = devm_kzalloc(dev, sizeof(*camclk), GFP_KERNEL);
+		if (!camclk)
+			return -ENOMEM;
+
+		snprintf(clk_name, sizeof(clk_name), "cam_clkout%d", nclocks);
+
+		init.name = clk_name;
+		init.ops = &cam_clk_ops;
+		init.flags = CLK_SET_RATE_PARENT;
+		init.parent_names = &cam_clk_p_names[nclocks];
+		init.num_parents = 1;
+		camclk->hw.init = &init;
+		camclk->fmd = fmd;
+
+		clk = devm_clk_register(dev, &camclk->hw);
+		if (IS_ERR(clk)) {
+			kfree(camclk);
+			return PTR_ERR(clk);
+		}
+		clk_provider->clks[nclocks] = clk;
+	}
+
+	clk_provider->clk_data.clks = clk_provider->clks;
+	clk_provider->clk_data.clk_num = nclocks;
+
+	return of_clk_add_provider(node, of_clk_src_onecell_get,
+					&clk_provider->clk_data);
+}
+#else
+#define fimc_md_register_clk_provider(fmd) (0)
+#endif
+
 static int fimc_md_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1465,16 +1547,24 @@ static int fimc_md_probe(struct platform_device *pdev)
 
 	fmd->use_isp = fimc_md_is_isp_available(dev->of_node);
 
+	ret = fimc_md_register_clk_provider(fmd);
+	if (ret < 0) {
+		v4l2_err(v4l2_dev, "clock provider registration failed\n");
+		return ret;
+	}
+
 	ret = v4l2_device_register(dev, &fmd->v4l2_dev);
 	if (ret < 0) {
 		v4l2_err(v4l2_dev, "Failed to register v4l2_device: %d\n", ret);
 		return ret;
 	}
+
 	ret = media_device_register(&fmd->media_dev);
 	if (ret < 0) {
 		v4l2_err(v4l2_dev, "Failed to register media device: %d\n", ret);
 		goto err_md;
 	}
+
 	ret = fimc_md_get_clocks(fmd);
 	if (ret)
 		goto err_clk;
@@ -1508,6 +1598,7 @@ static int fimc_md_probe(struct platform_device *pdev)
 	ret = fimc_md_create_links(fmd);
 	if (ret)
 		goto err_unlock;
+
 	ret = v4l2_device_register_subdev_nodes(&fmd->v4l2_dev);
 	if (ret)
 		goto err_unlock;
@@ -1528,6 +1619,7 @@ err_clk:
 	media_device_unregister(&fmd->media_dev);
 err_md:
 	v4l2_device_unregister(&fmd->v4l2_dev);
+	fimc_md_unregister_clk_provider(fmd);
 	return ret;
 }
 
diff --git a/drivers/media/platform/exynos4-is/media-dev.h b/drivers/media/platform/exynos4-is/media-dev.h
index 62599fd..09cc6ca 100644
--- a/drivers/media/platform/exynos4-is/media-dev.h
+++ b/drivers/media/platform/exynos4-is/media-dev.h
@@ -10,6 +10,7 @@
 #define FIMC_MDEVICE_H_
 
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/platform_device.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
@@ -105,6 +106,7 @@ struct fimc_sensor_info {
  * @pinctrl: camera port pinctrl handle
  * @state_default: pinctrl default state handle
  * @state_idle: pinctrl idle state handle
+ * @cam_clk_provider: CAMCLK clock provider structure
  * @user_subdev_api: true if subdevs are not configured by the host driver
  * @slock: spinlock protecting @sensor array
  */
@@ -122,13 +124,20 @@ struct fimc_md {
 	struct media_device media_dev;
 	struct v4l2_device v4l2_dev;
 	struct platform_device *pdev;
+
 	struct fimc_pinctrl {
 		struct pinctrl *pinctrl;
 		struct pinctrl_state *state_default;
 		struct pinctrl_state *state_idle;
 	} pinctl;
-	bool user_subdev_api;
 
+	struct cam_clk_provider {
+		struct clk *clks[FIMC_MAX_CAMCLKS];
+		struct clk_onecell_data clk_data;
+		struct device_node *of_node;
+	} clk_provider;
+
+	bool user_subdev_api;
 	spinlock_t slock;
 	struct list_head pipelines;
 };
@@ -163,8 +172,16 @@ static inline bool fimc_md_is_isp_available(struct device_node *node)
 	node = of_get_child_by_name(node, FIMC_IS_OF_NODE_NAME);
 	return node ? of_device_is_available(node) : false;
 }
+
+static inline void fimc_md_unregister_clk_provider(struct fimc_md *fmd)
+{
+	if (fmd->clk_provider.of_node)
+		of_clk_del_provider(fmd->clk_provider.of_node);
+}
 #else
+
 #define fimc_md_is_isp_available(node) (false)
+#define fimc_md_unregister_clk_provider(fmd) (0)
 #endif /* CONFIG_OF */
 
 static inline struct v4l2_subdev *__fimc_md_get_subdev(
-- 
1.7.9.5

[REVIEW PATCH 5/6] exynos4-is: Use external s5k6a3 sensor driver

[Sylwester Nawrocki]

This patch removes the common fimc-is-sensor driver for image sensors
that are normally controlled by the FIMC-IS firmware. The FIMC-IS
driver now contains only a table of properties specific to each sensor.
The sensor properties required for the ISP's firmware are parsed from
device tree and retrieved from the internal table, which is selected
based on the compatible property of an image sensor.

To use the Exynos4x12 internal ISP the S5K6A3 sensor driver (drivers/
media/i2c/s5k6a3.c) is now required.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/exynos4-is/fimc-is-regs.c   |    2 +-
 drivers/media/platform/exynos4-is/fimc-is-sensor.c |  285 +-------------------
 drivers/media/platform/exynos4-is/fimc-is-sensor.h |   49 +---
 drivers/media/platform/exynos4-is/fimc-is.c        |   96 +++----
 drivers/media/platform/exynos4-is/fimc-is.h        |    4 +-
 5 files changed, 57 insertions(+), 379 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-is-regs.c b/drivers/media/platform/exynos4-is/fimc-is-regs.c
index 63c68ec..3e51aa6 100644
--- a/drivers/media/platform/exynos4-is/fimc-is-regs.c
+++ b/drivers/media/platform/exynos4-is/fimc-is-regs.c
@@ -136,7 +136,7 @@ void fimc_is_hw_set_sensor_num(struct fimc_is *is)
 	mcuctl_write(IH_REPLY_DONE, is, MCUCTL_REG_ISSR(0));
 	mcuctl_write(is->sensor_index, is, MCUCTL_REG_ISSR(1));
 	mcuctl_write(IHC_GET_SENSOR_NUM, is, MCUCTL_REG_ISSR(2));
-	mcuctl_write(FIMC_IS_SENSOR_NUM, is, MCUCTL_REG_ISSR(3));
+	mcuctl_write(FIMC_IS_SENSORS_NUM, is, MCUCTL_REG_ISSR(3));
 }
 
 void fimc_is_hw_close_sensor(struct fimc_is *is, unsigned int index)
diff --git a/drivers/media/platform/exynos4-is/fimc-is-sensor.c b/drivers/media/platform/exynos4-is/fimc-is-sensor.c
index 6647421..10e82e2 100644
--- a/drivers/media/platform/exynos4-is/fimc-is-sensor.c
+++ b/drivers/media/platform/exynos4-is/fimc-is-sensor.c
@@ -2,276 +2,21 @@
  * Samsung EXYNOS4x12 FIMC-IS (Imaging Subsystem) driver
  *
  * Copyright (C) 2013 Samsung Electronics Co., Ltd.
- *
  * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
-#include <linux/delay.h>
-#include <linux/device.h>
-#include <linux/errno.h>
-#include <linux/gpio.h>
-#include <linux/i2c.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/of_gpio.h>
-#include <linux/pm_runtime.h>
-#include <linux/regulator/consumer.h>
-#include <linux/slab.h>
-#include <media/v4l2-subdev.h>
 
-#include "fimc-is.h"
 #include "fimc-is-sensor.h"
 
-#define DRIVER_NAME "FIMC-IS-SENSOR"
-
-static const char * const sensor_supply_names[] = {
-	"svdda",
-	"svddio",
-};
-
-static const struct v4l2_mbus_framefmt fimc_is_sensor_formats[] = {
-	{
-		.code = V4L2_MBUS_FMT_SGRBG10_1X10,
-		.colorspace = V4L2_COLORSPACE_SRGB,
-		.field = V4L2_FIELD_NONE,
-	}
-};
-
-static const struct v4l2_mbus_framefmt *find_sensor_format(
-	struct v4l2_mbus_framefmt *mf)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(fimc_is_sensor_formats); i++)
-		if (mf->code == fimc_is_sensor_formats[i].code)
-			return &fimc_is_sensor_formats[i];
-
-	return &fimc_is_sensor_formats[0];
-}
-
-static int fimc_is_sensor_enum_mbus_code(struct v4l2_subdev *sd,
-				  struct v4l2_subdev_fh *fh,
-				  struct v4l2_subdev_mbus_code_enum *code)
-{
-	if (code->index >= ARRAY_SIZE(fimc_is_sensor_formats))
-		return -EINVAL;
-
-	code->code = fimc_is_sensor_formats[code->index].code;
-	return 0;
-}
-
-static void fimc_is_sensor_try_format(struct fimc_is_sensor *sensor,
-				      struct v4l2_mbus_framefmt *mf)
-{
-	const struct sensor_drv_data *dd = sensor->drvdata;
-	const struct v4l2_mbus_framefmt *fmt;
-
-	fmt = find_sensor_format(mf);
-	mf->code = fmt->code;
-	v4l_bound_align_image(&mf->width, 16 + 8, dd->width, 0,
-			      &mf->height, 12 + 8, dd->height, 0, 0);
-}
-
-static struct v4l2_mbus_framefmt *__fimc_is_sensor_get_format(
-		struct fimc_is_sensor *sensor, struct v4l2_subdev_fh *fh,
-		u32 pad, enum v4l2_subdev_format_whence which)
-{
-	if (which == V4L2_SUBDEV_FORMAT_TRY)
-		return fh ? v4l2_subdev_get_try_format(fh, pad) : NULL;
-
-	return &sensor->format;
-}
-
-static int fimc_is_sensor_set_fmt(struct v4l2_subdev *sd,
-				  struct v4l2_subdev_fh *fh,
-				  struct v4l2_subdev_format *fmt)
-{
-	struct fimc_is_sensor *sensor = sd_to_fimc_is_sensor(sd);
-	struct v4l2_mbus_framefmt *mf;
-
-	fimc_is_sensor_try_format(sensor, &fmt->format);
-
-	mf = __fimc_is_sensor_get_format(sensor, fh, fmt->pad, fmt->which);
-	if (mf) {
-		mutex_lock(&sensor->lock);
-		if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-			*mf = fmt->format;
-		mutex_unlock(&sensor->lock);
-	}
-	return 0;
-}
-
-static int fimc_is_sensor_get_fmt(struct v4l2_subdev *sd,
-				  struct v4l2_subdev_fh *fh,
-				  struct v4l2_subdev_format *fmt)
-{
-	struct fimc_is_sensor *sensor = sd_to_fimc_is_sensor(sd);
-	struct v4l2_mbus_framefmt *mf;
-
-	mf = __fimc_is_sensor_get_format(sensor, fh, fmt->pad, fmt->which);
-
-	mutex_lock(&sensor->lock);
-	fmt->format = *mf;
-	mutex_unlock(&sensor->lock);
-	return 0;
-}
-
-static struct v4l2_subdev_pad_ops fimc_is_sensor_pad_ops = {
-	.enum_mbus_code	= fimc_is_sensor_enum_mbus_code,
-	.get_fmt	= fimc_is_sensor_get_fmt,
-	.set_fmt	= fimc_is_sensor_set_fmt,
-};
-
-static int fimc_is_sensor_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
-{
-	struct v4l2_mbus_framefmt *format = v4l2_subdev_get_try_format(fh, 0);
-
-	*format		= fimc_is_sensor_formats[0];
-	format->width	= FIMC_IS_SENSOR_DEF_PIX_WIDTH;
-	format->height	= FIMC_IS_SENSOR_DEF_PIX_HEIGHT;
-
-	return 0;
-}
-
-static const struct v4l2_subdev_internal_ops fimc_is_sensor_sd_internal_ops = {
-	.open = fimc_is_sensor_open,
-};
-
-static int fimc_is_sensor_s_power(struct v4l2_subdev *sd, int on)
-{
-	struct fimc_is_sensor *sensor = sd_to_fimc_is_sensor(sd);
-	int gpio = sensor->gpio_reset;
-	int ret;
-
-	if (on) {
-		ret = pm_runtime_get(sensor->dev);
-		if (ret < 0)
-			return ret;
-
-		ret = regulator_bulk_enable(SENSOR_NUM_SUPPLIES,
-					    sensor->supplies);
-		if (ret < 0) {
-			pm_runtime_put(sensor->dev);
-			return ret;
-		}
-		if (gpio_is_valid(gpio)) {
-			gpio_set_value(gpio, 1);
-			usleep_range(600, 800);
-			gpio_set_value(gpio, 0);
-			usleep_range(10000, 11000);
-			gpio_set_value(gpio, 1);
-		}
-
-		/* A delay needed for the sensor initialization. */
-		msleep(20);
-	} else {
-		if (gpio_is_valid(gpio))
-			gpio_set_value(gpio, 0);
-
-		ret = regulator_bulk_disable(SENSOR_NUM_SUPPLIES,
-					     sensor->supplies);
-		if (!ret)
-			pm_runtime_put(sensor->dev);
-	}
-
-	pr_info("%s:%d: on: %d, ret: %d\n", __func__, __LINE__, on, ret);
-
-	return ret;
-}
-
-static struct v4l2_subdev_core_ops fimc_is_sensor_core_ops = {
-	.s_power = fimc_is_sensor_s_power,
-};
-
-static struct v4l2_subdev_ops fimc_is_sensor_subdev_ops = {
-	.core = &fimc_is_sensor_core_ops,
-	.pad = &fimc_is_sensor_pad_ops,
-};
-
-static const struct of_device_id fimc_is_sensor_of_match[];
-
-static int fimc_is_sensor_probe(struct i2c_client *client,
-				const struct i2c_device_id *id)
-{
-	struct device *dev = &client->dev;
-	struct fimc_is_sensor *sensor;
-	const struct of_device_id *of_id;
-	struct v4l2_subdev *sd;
-	int gpio, i, ret;
-
-	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
-	if (!sensor)
-		return -ENOMEM;
-
-	mutex_init(&sensor->lock);
-	sensor->gpio_reset = -EINVAL;
-
-	gpio = of_get_gpio_flags(dev->of_node, 0, NULL);
-	if (gpio_is_valid(gpio)) {
-		ret = devm_gpio_request_one(dev, gpio, GPIOF_OUT_INIT_LOW,
-							DRIVER_NAME);
-		if (ret < 0)
-			return ret;
-	}
-	sensor->gpio_reset = gpio;
-
-	for (i = 0; i < SENSOR_NUM_SUPPLIES; i++)
-		sensor->supplies[i].supply = sensor_supply_names[i];
-
-	ret = devm_regulator_bulk_get(&client->dev, SENSOR_NUM_SUPPLIES,
-				      sensor->supplies);
-	if (ret < 0)
-		return ret;
-
-	of_id = of_match_node(fimc_is_sensor_of_match, dev->of_node);
-	if (!of_id)
-		return -ENODEV;
-
-	sensor->drvdata = of_id->data;
-	sensor->dev = dev;
-
-	sd = &sensor->subdev;
-	v4l2_i2c_subdev_init(sd, client, &fimc_is_sensor_subdev_ops);
-	snprintf(sd->name, sizeof(sd->name), sensor->drvdata->subdev_name);
-	sensor->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-
-	sensor->format.code = fimc_is_sensor_formats[0].code;
-	sensor->format.width = FIMC_IS_SENSOR_DEF_PIX_WIDTH;
-	sensor->format.height = FIMC_IS_SENSOR_DEF_PIX_HEIGHT;
-
-	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
-	ret = media_entity_init(&sd->entity, 1, &sensor->pad, 0);
-	if (ret < 0)
-		return ret;
-
-	pm_runtime_no_callbacks(dev);
-	pm_runtime_enable(dev);
-
-	return ret;
-}
-
-static int fimc_is_sensor_remove(struct i2c_client *client)
-{
-	struct v4l2_subdev *sd = i2c_get_clientdata(client);
-	media_entity_cleanup(&sd->entity);
-	return 0;
-}
-
-static const struct i2c_device_id fimc_is_sensor_ids[] = {
-	{ }
-};
-
 static const struct sensor_drv_data s5k6a3_drvdata = {
 	.id		= FIMC_IS_SENSOR_ID_S5K6A3,
-	.subdev_name	= "S5K6A3",
-	.width		= S5K6A3_SENSOR_WIDTH,
-	.height		= S5K6A3_SENSOR_HEIGHT,
+	.open_timeout	= S5K6A3_OPEN_TIMEOUT,
 };
 
-static const struct of_device_id fimc_is_sensor_of_match[] = {
+static const struct of_device_id fimc_is_sensor_of_ids[] = {
 	{
 		.compatible	= "samsung,s5k6a3",
 		.data		= &s5k6a3_drvdata,
@@ -279,27 +24,11 @@ static const struct of_device_id fimc_is_sensor_of_match[] = {
 	{  }
 };
 
-static struct i2c_driver fimc_is_sensor_driver = {
-	.driver = {
-		.of_match_table	= fimc_is_sensor_of_match,
-		.name		= DRIVER_NAME,
-		.owner		= THIS_MODULE,
-	},
-	.probe		= fimc_is_sensor_probe,
-	.remove		= fimc_is_sensor_remove,
-	.id_table	= fimc_is_sensor_ids,
-};
-
-int fimc_is_register_sensor_driver(void)
+const struct sensor_drv_data *fimc_is_sensor_get_drvdata(
+			struct device_node *node)
 {
-	return i2c_add_driver(&fimc_is_sensor_driver);
-}
+	const struct of_device_id *of_id;
 
-void fimc_is_unregister_sensor_driver(void)
-{
-	i2c_del_driver(&fimc_is_sensor_driver);
+	of_id = of_match_node(fimc_is_sensor_of_ids, node);
+	return of_id ? of_id->data : NULL;
 }
-
-MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
-MODULE_DESCRIPTION("Exynos4x12 FIMC-IS image sensor subdev driver");
-MODULE_LICENSE("GPL");
diff --git a/drivers/media/platform/exynos4-is/fimc-is-sensor.h b/drivers/media/platform/exynos4-is/fimc-is-sensor.h
index 6036d49..173ccff 100644
--- a/drivers/media/platform/exynos4-is/fimc-is-sensor.h
+++ b/drivers/media/platform/exynos4-is/fimc-is-sensor.h
@@ -13,24 +13,13 @@
 #ifndef FIMC_IS_SENSOR_H_
 #define FIMC_IS_SENSOR_H_
 
-#include <linux/clk.h>
-#include <linux/device.h>
-#include <linux/kernel.h>
-#include <linux/platform_device.h>
-#include <linux/regulator/consumer.h>
-#include <linux/videodev2.h>
-#include <media/v4l2-subdev.h>
-
-#define FIMC_IS_SENSOR_OPEN_TIMEOUT	2000 /* ms */
-
-#define FIMC_IS_SENSOR_DEF_PIX_WIDTH	1296
-#define FIMC_IS_SENSOR_DEF_PIX_HEIGHT	732
+#include <linux/of.h>
+#include <linux/types.h>
 
+#define S5K6A3_OPEN_TIMEOUT		2000 /* ms */
 #define S5K6A3_SENSOR_WIDTH		1392
 #define S5K6A3_SENSOR_HEIGHT		1392
 
-#define SENSOR_NUM_SUPPLIES		2
-
 enum fimc_is_sensor_id {
 	FIMC_IS_SENSOR_ID_S5K3H2 = 1,
 	FIMC_IS_SENSOR_ID_S5K6A3,
@@ -45,45 +34,23 @@ enum fimc_is_sensor_id {
 
 struct sensor_drv_data {
 	enum fimc_is_sensor_id id;
-	const char * const subdev_name;
-	unsigned int width;
-	unsigned int height;
+	/* sensor open timeout in ms */
+	unsigned short open_timeout;
 };
 
 /**
  * struct fimc_is_sensor - fimc-is sensor data structure
- * @dev: pointer to this I2C client device structure
- * @subdev: the image sensor's v4l2 subdev
- * @pad: subdev media source pad
- * @supplies: image sensor's voltage regulator supplies
- * @gpio_reset: GPIO connected to the sensor's reset pin
  * @drvdata: a pointer to the sensor's parameters data structure
  * @i2c_bus: ISP I2C bus index (0...1)
  * @test_pattern: true to enable video test pattern
- * @lock: mutex protecting the structure's members below
- * @format: media bus format at the sensor's source pad
  */
 struct fimc_is_sensor {
-	struct device *dev;
-	struct v4l2_subdev subdev;
-	struct media_pad pad;
-	struct regulator_bulk_data supplies[SENSOR_NUM_SUPPLIES];
-	int gpio_reset;
 	const struct sensor_drv_data *drvdata;
 	unsigned int i2c_bus;
-	bool test_pattern;
-
-	struct mutex lock;
-	struct v4l2_mbus_framefmt format;
+	u8 test_pattern;
 };
 
-static inline
-struct fimc_is_sensor *sd_to_fimc_is_sensor(struct v4l2_subdev *sd)
-{
-	return container_of(sd, struct fimc_is_sensor, subdev);
-}
-
-int fimc_is_register_sensor_driver(void);
-void fimc_is_unregister_sensor_driver(void);
+const struct sensor_drv_data *fimc_is_sensor_get_drvdata(
+				struct device_node *node);
 
 #endif /* FIMC_IS_SENSOR_H_ */
diff --git a/drivers/media/platform/exynos4-is/fimc-is.c b/drivers/media/platform/exynos4-is/fimc-is.c
index 967f6a9..fbfae84 100644
--- a/drivers/media/platform/exynos4-is/fimc-is.c
+++ b/drivers/media/platform/exynos4-is/fimc-is.c
@@ -161,78 +161,67 @@ static void fimc_is_disable_clocks(struct fimc_is *is)
 	}
 }
 
-static int fimc_is_parse_sensor_config(struct fimc_is_sensor *sensor,
-				       struct device_node *np)
+static int fimc_is_parse_sensor_config(struct fimc_is *is, unsigned int index,
+						struct device_node *node)
 {
+	struct fimc_is_sensor *sensor = &is->sensor[index];
 	u32 tmp = 0;
 	int ret;
 
-	np = v4l2_of_get_next_endpoint(np, NULL);
-	if (!np)
+	sensor->drvdata = fimc_is_sensor_get_drvdata(node);
+	if (!sensor->drvdata) {
+		dev_err(&is->pdev->dev, "no driver data found for: %s\n",
+							 node->full_name);
+		return -EINVAL;
+	}
+
+	node = v4l2_of_get_next_endpoint(node, NULL);
+	if (!node)
 		return -ENXIO;
-	np = v4l2_of_get_remote_port(np);
-	if (!np)
+
+	node = v4l2_of_get_remote_port(node);
+	if (!node)
 		return -ENXIO;
 
 	/* Use MIPI-CSIS channel id to determine the ISP I2C bus index. */
-	ret = of_property_read_u32(np, "reg", &tmp);
-	sensor->i2c_bus = tmp - FIMC_INPUT_MIPI_CSI2_0;
+	ret = of_property_read_u32(node, "reg", &tmp);
+	if (ret < 0) {
+		dev_err(&is->pdev->dev, "reg property not found at: %s\n",
+							 node->full_name);
+		return ret;
+	}
 
-	return ret;
+	sensor->i2c_bus = tmp - FIMC_INPUT_MIPI_CSI2_0;
+	return 0;
 }
 
 static int fimc_is_register_subdevs(struct fimc_is *is)
 {
-	struct device_node *adapter, *child;
-	int ret;
+	struct device_node *i2c_bus, *child;
+	int ret, index = 0;
 
 	ret = fimc_isp_subdev_create(&is->isp);
 	if (ret < 0)
 		return ret;
 
-	for_each_compatible_node(adapter, NULL, FIMC_IS_I2C_COMPATIBLE) {
-		if (!of_find_device_by_node(adapter)) {
-			of_node_put(adapter);
-			return -EPROBE_DEFER;
-		}
-
-		for_each_available_child_of_node(adapter, child) {
-			struct i2c_client *client;
-			struct v4l2_subdev *sd;
-
-			client = of_find_i2c_device_by_node(child);
-			if (!client)
-				goto e_retry;
-
-			sd = i2c_get_clientdata(client);
-			if (!sd)
-				goto e_retry;
 
-			/* FIXME: Add support for multiple sensors. */
-			if (WARN_ON(is->sensor))
-				continue;
+	for_each_compatible_node(i2c_bus, NULL, FIMC_IS_I2C_COMPATIBLE) {
+		for_each_available_child_of_node(i2c_bus, child) {
+			ret = fimc_is_parse_sensor_config(is, index, child);
 
-			is->sensor = sd_to_fimc_is_sensor(sd);
-
-			if (fimc_is_parse_sensor_config(is->sensor, child)) {
-				dev_warn(&is->pdev->dev, "DT parse error: %s\n",
-							 child->full_name);
+			if (ret < 0 || index >= FIMC_IS_SENSORS_NUM) {
+				of_node_put(child);
+				return ret;
 			}
-			pr_debug("%s(): registered subdev: %p\n",
-				 __func__, sd->name);
+			index++;
 		}
 	}
 	return 0;
-
-e_retry:
-	of_node_put(child);
-	return -EPROBE_DEFER;
 }
 
 static int fimc_is_unregister_subdevs(struct fimc_is *is)
 {
 	fimc_isp_subdev_destroy(&is->isp);
-	is->sensor = NULL;
 	return 0;
 }
 
@@ -647,7 +636,7 @@ static int fimc_is_hw_open_sensor(struct fimc_is *is,
 	fimc_is_hw_set_intgr0_gd0(is);
 
 	return fimc_is_wait_event(is, IS_ST_OPEN_SENSOR, 1,
-				  FIMC_IS_SENSOR_OPEN_TIMEOUT);
+				  sensor->drvdata->open_timeout);
 }
 
 
@@ -661,8 +650,8 @@ int fimc_is_hw_initialize(struct fimc_is *is)
 	u32 prev_id;
 	int i, ret;
 
-	/* Sensor initialization. */
-	ret = fimc_is_hw_open_sensor(is, is->sensor);
+	/* Sensor initialization. Only one sensor is currently supported. */
+	ret = fimc_is_hw_open_sensor(is, &is->sensor[0]);
 	if (ret < 0)
 		return ret;
 
@@ -962,27 +951,20 @@ static int fimc_is_module_init(void)
 {
 	int ret;
 
-	ret = fimc_is_register_sensor_driver();
-	if (ret < 0)
-		return ret;
-
 	ret = fimc_is_register_i2c_driver();
 	if (ret < 0)
-		goto err_sens;
+		return ret;
 
 	ret = platform_driver_register(&fimc_is_driver);
-	if (!ret)
-		return ret;
 
-	fimc_is_unregister_i2c_driver();
-err_sens:
-	fimc_is_unregister_sensor_driver();
+	if (ret < 0)
+		fimc_is_unregister_i2c_driver();
+
 	return ret;
 }
 
 static void fimc_is_module_exit(void)
 {
-	fimc_is_unregister_sensor_driver();
 	fimc_is_unregister_i2c_driver();
 	platform_driver_unregister(&fimc_is_driver);
 }
diff --git a/drivers/media/platform/exynos4-is/fimc-is.h b/drivers/media/platform/exynos4-is/fimc-is.h
index 61bb012..01f802f 100644
--- a/drivers/media/platform/exynos4-is/fimc-is.h
+++ b/drivers/media/platform/exynos4-is/fimc-is.h
@@ -39,7 +39,7 @@
 #define FIMC_IS_FW_LOAD_TIMEOUT		1000 /* ms */
 #define FIMC_IS_POWER_ON_TIMEOUT	1000 /* us */
 
-#define FIMC_IS_SENSOR_NUM		2
+#define FIMC_IS_SENSORS_NUM		2
 
 /* Memory definitions */
 #define FIMC_IS_CPU_MEM_SIZE		(0xa00000)
@@ -253,7 +253,7 @@ struct fimc_is {
 	struct firmware			*f_w;
 
 	struct fimc_isp			isp;
-	struct fimc_is_sensor		*sensor;
+	struct fimc_is_sensor		sensor[FIMC_IS_SENSORS_NUM];
 	struct fimc_is_setfile		setfile;
 
 	struct vb2_alloc_ctx		*alloc_ctx;
-- 
1.7.9.5

[REVIEW PATCH 6/6] exynos4-is: Add support for asynchronous sensor subddevs registration

[Sylwester Nawrocki]

Add support registering external sensor subdevs using the v4l2-async API.
The async API is used only for sensor subdevs and only for platforms
instatiated from Device Tree.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/platform/exynos4-is/media-dev.c |  163 ++++++++++++++-----------
 drivers/media/platform/exynos4-is/media-dev.h |   12 +-
 2 files changed, 100 insertions(+), 75 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index 346e1e0..280e819 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -27,6 +27,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/types.h>
 #include <linux/slab.h>
+#include <media/v4l2-async.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-of.h>
 #include <media/media-device.h>
@@ -221,6 +222,7 @@ static int __fimc_pipeline_open(struct exynos_media_pipeline *ep,
 		if (ret < 0)
 			return ret;
 	}
+
 	ret = fimc_md_set_camclk(sd, true);
 	if (ret < 0)
 		goto err_wbclk;
@@ -395,63 +397,6 @@ static void fimc_md_unregister_sensor(struct v4l2_subdev *sd)
 }
 
 #ifdef CONFIG_OF
-/* Register I2C client subdev associated with @node. */
-static int fimc_md_of_add_sensor(struct fimc_md *fmd,
-				 struct device_node *node, int index)
-{
-	struct fimc_sensor_info *si;
-	struct i2c_client *client;
-	struct v4l2_subdev *sd;
-	int ret;
-
-	if (WARN_ON(index >= ARRAY_SIZE(fmd->sensor)))
-		return -EINVAL;
-	si = &fmd->sensor[index];
-
-	client = of_find_i2c_device_by_node(node);
-	if (!client)
-		return -EPROBE_DEFER;
-
-	device_lock(&client->dev);
-
-	if (!client->driver ||
-	    !try_module_get(client->driver->driver.owner)) {
-		ret = -EPROBE_DEFER;
-		v4l2_info(&fmd->v4l2_dev, "No driver found for %s\n",
-						node->full_name);
-		goto dev_put;
-	}
-
-	/* Enable sensor's master clock */
-	ret = __fimc_md_set_camclk(fmd, &si->pdata, true);
-	if (ret < 0)
-		goto mod_put;
-	sd = i2c_get_clientdata(client);
-
-	ret = v4l2_device_register_subdev(&fmd->v4l2_dev, sd);
-	__fimc_md_set_camclk(fmd, &si->pdata, false);
-	if (ret < 0)
-		goto mod_put;
-
-	v4l2_set_subdev_hostdata(sd, &si->pdata);
-	if (si->pdata.fimc_bus_type == FIMC_BUS_TYPE_ISP_WRITEBACK)
-		sd->grp_id = GRP_ID_FIMC_IS_SENSOR;
-	else
-		sd->grp_id = GRP_ID_SENSOR;
-
-	si->subdev = sd;
-	v4l2_info(&fmd->v4l2_dev, "Registered sensor subdevice: %s (%d)\n",
-		  sd->name, fmd->num_sensors);
-	fmd->num_sensors++;
-
-mod_put:
-	module_put(client->driver->driver.owner);
-dev_put:
-	device_unlock(&client->dev);
-	put_device(&client->dev);
-	return ret;
-}
-
 /* Parse port node and register as a sub-device any sensor specified there. */
 static int fimc_md_parse_port_node(struct fimc_md *fmd,
 				   struct device_node *port,
@@ -460,7 +405,6 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
 	struct device_node *rem, *ep, *np;
 	struct fimc_source_info *pd;
 	struct v4l2_of_endpoint endpoint;
-	int ret;
 	u32 val;
 
 	pd = &fmd->sensor[index].pdata;
@@ -527,10 +471,17 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
 	else
 		pd->fimc_bus_type = pd->sensor_bus_type;
 
-	ret = fimc_md_of_add_sensor(fmd, rem, index);
-	of_node_put(rem);
+	if (WARN_ON(index >= ARRAY_SIZE(fmd->sensor)))
+		return -EINVAL;
 
-	return ret;
+	fmd->sensor[index].asd.match_type = V4L2_ASYNC_MATCH_OF;
+	fmd->sensor[index].asd.match.of.node = rem;
+	fmd->async_subdevs[index] = &fmd->sensor[index].asd;
+
+	fmd->num_sensors++;
+
+	of_node_put(rem);
+	return 0;
 }
 
 /* Register all SoC external sub-devices */
@@ -1225,6 +1176,14 @@ static int __fimc_md_set_camclk(struct fimc_md *fmd,
 	struct fimc_camclk_info *camclk;
 	int ret = 0;
 
+	/*
+	 * When device tree is used the sensor drivers are supposed to
+	 * control the clock themselves. This whole function will be
+	 * removed once S5PV210 platform is converted to the device tree.
+	 */
+	if (fmd->pdev->dev.of_node)
+		return 0;
+
 	if (WARN_ON(si->clk_id >= FIMC_MAX_CAMCLKS) || !fmd || !fmd->pmf)
 		return -EINVAL;
 
@@ -1520,6 +1479,56 @@ static int fimc_md_register_clk_provider(struct fimc_md *fmd)
 #define fimc_md_register_clk_provider(fmd) (0)
 #endif
 
+static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
+				 struct v4l2_subdev *subdev,
+				 struct v4l2_async_subdev *asd)
+{
+	struct fimc_md *fmd = notifier_to_fimc_md(notifier);
+	struct fimc_sensor_info *si = NULL;
+	int i;
+
+	/* Find platform data for this sensor subdev */
+	for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
+		if (fmd->sensor[i].asd.match.of.node == subdev->dev->of_node)
+			si = &fmd->sensor[i];
+
+	if (si == NULL)
+		return -EINVAL;
+
+	v4l2_set_subdev_hostdata(subdev, &si->pdata);
+
+	if (si->pdata.fimc_bus_type == FIMC_BUS_TYPE_ISP_WRITEBACK)
+		subdev->grp_id = GRP_ID_FIMC_IS_SENSOR;
+	else
+		subdev->grp_id = GRP_ID_SENSOR;
+
+	si->subdev = subdev;
+
+	v4l2_info(&fmd->v4l2_dev, "Registered sensor subdevice: %s (%d)\n",
+		  subdev->name, fmd->num_sensors);
+
+	fmd->num_sensors++;
+
+	return 0;
+}
+
+static int subdev_notifier_complete(struct v4l2_async_notifier *notifier)
+{
+	struct fimc_md *fmd = notifier_to_fimc_md(notifier);
+	int ret;
+
+	mutex_lock(&fmd->media_dev.graph_mutex);
+
+	ret = fimc_md_create_links(fmd);
+	if (ret < 0)
+		goto unlock;
+
+	ret = v4l2_device_register_subdev_nodes(&fmd->v4l2_dev);
+unlock:
+	mutex_unlock(&fmd->media_dev.graph_mutex);
+	return ret;
+}
+
 static int fimc_md_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1571,9 +1580,6 @@ static int fimc_md_probe(struct platform_device *pdev)
 
 	fmd->user_subdev_api = (dev->of_node != NULL);
 
-	/* Protect the media graph while we're registering entities */
-	mutex_lock(&fmd->media_dev.graph_mutex);
-
 	ret = fimc_md_get_pinctrl(fmd);
 	if (ret < 0) {
 		if (ret != EPROBE_DEFER)
@@ -1581,6 +1587,11 @@ static int fimc_md_probe(struct platform_device *pdev)
 		goto err_unlock;
 	}
 
+	platform_set_drvdata(pdev, fmd);
+
+	/* Protect the media graph while we're registering entities */
+	mutex_lock(&fmd->media_dev.graph_mutex);
+
 	if (dev->of_node)
 		ret = fimc_md_register_of_platform_entities(fmd, dev->of_node);
 	else
@@ -1595,20 +1606,23 @@ static int fimc_md_probe(struct platform_device *pdev)
 			goto err_unlock;
 	}
 
-	ret = fimc_md_create_links(fmd);
+	ret = device_create_file(&pdev->dev, &dev_attr_subdev_conf_mode);
 	if (ret)
 		goto err_unlock;
 
-	ret = v4l2_device_register_subdev_nodes(&fmd->v4l2_dev);
-	if (ret)
-		goto err_unlock;
+	mutex_unlock(&fmd->media_dev.graph_mutex);
 
-	ret = device_create_file(&pdev->dev, &dev_attr_subdev_conf_mode);
+	fmd->subdev_notifier.subdevs = fmd->async_subdevs;
+	fmd->subdev_notifier.num_subdevs = fmd->num_sensors;
+	fmd->subdev_notifier.bound = subdev_notifier_bound;
+	fmd->subdev_notifier.complete = subdev_notifier_complete;
+	fmd->num_sensors = 0;
+
+	ret = v4l2_async_notifier_register(&fmd->v4l2_dev,
+					   &fmd->subdev_notifier);
 	if (ret)
-		goto err_unlock;
+		goto err_clk;
 
-	platform_set_drvdata(pdev, fmd);
-	mutex_unlock(&fmd->media_dev.graph_mutex);
 	return 0;
 
 err_unlock:
@@ -1627,13 +1641,14 @@ static int fimc_md_remove(struct platform_device *pdev)
 {
 	struct fimc_md *fmd = platform_get_drvdata(pdev);
 
-	if (!fmd)
-		return 0;
+	v4l2_async_notifier_unregister(&fmd->subdev_notifier);
+
 	device_remove_file(&pdev->dev, &dev_attr_subdev_conf_mode);
 	fimc_md_unregister_entities(fmd);
 	fimc_md_pipelines_free(fmd);
 	media_device_unregister(&fmd->media_dev);
 	fimc_md_put_clocks(fmd);
+
 	return 0;
 }
 
diff --git a/drivers/media/platform/exynos4-is/media-dev.h b/drivers/media/platform/exynos4-is/media-dev.h
index 09cc6ca..276ad80 100644
--- a/drivers/media/platform/exynos4-is/media-dev.h
+++ b/drivers/media/platform/exynos4-is/media-dev.h
@@ -32,7 +32,7 @@
 
 #define PINCTRL_STATE_IDLE	"idle"
 
-#define FIMC_MAX_SENSORS	8
+#define FIMC_MAX_SENSORS	4
 #define FIMC_MAX_CAMCLKS	2
 
 /* LCD/ISP Writeback clocks (PIXELASYNCMx) */
@@ -79,6 +79,7 @@ struct fimc_camclk_info {
 /**
  * struct fimc_sensor_info - image data source subdev information
  * @pdata: sensor's atrributes passed as media device's platform data
+ * @asd: asynchronous subdev registration data structure
  * @subdev: image sensor v4l2 subdev
  * @host: fimc device the sensor is currently linked to
  *
@@ -86,6 +87,7 @@ struct fimc_camclk_info {
  */
 struct fimc_sensor_info {
 	struct fimc_source_info pdata;
+	struct v4l2_async_subdev asd;
 	struct v4l2_subdev *subdev;
 	struct fimc_dev *host;
 };
@@ -137,6 +139,9 @@ struct fimc_md {
 		struct device_node *of_node;
 	} clk_provider;
 
+	struct v4l2_async_notifier subdev_notifier;
+	struct v4l2_async_subdev *async_subdevs[FIMC_MAX_SENSORS];
+
 	bool user_subdev_api;
 	spinlock_t slock;
 	struct list_head pipelines;
@@ -154,6 +159,11 @@ static inline struct fimc_md *entity_to_fimc_mdev(struct media_entity *me)
 		container_of(me->parent, struct fimc_md, media_dev);
 }
 
+static inline struct fimc_md *notifier_to_fimc_md(struct v4l2_async_notifier *n)
+{
+	return container_of(n, struct fimc_md, subdev_notifier);
+}
+
 static inline void fimc_md_graph_lock(struct exynos_video_entity *ve)
 {
 	mutex_lock(&ve->vdev.entity.parent->graph_mutex);
-- 
1.7.9.5

Re: [REVIEW PATCH 0/6] exynos4-is: Asynchronous subdev registration support

[Arun Kumar K]

Hi Sylwester,

On Wed, Jul 24, 2013 at 12:09 AM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> This patch series is a refactoring of the exynos4-is driver to get rid
> of the common fimc-is-sensor driver and to adapt it to use "standard"
> sensor subdev drivers, one per each image sensor type.
> Then a clock provider is added to the exynos4-is driver and the s5k6a3
> subdev is modified to use one of the clocks registered by exynos4-is.
>
> Arun, I think you could reuse the s5k6a3 sensor for your work on the
> Exynos5 FIMC-IS driver. One advantage of separate sensor drivers is
> that the power on/off sequences can be written specifically for each
> sensor. We are probably going to need such sequences per board in
> future. Also having the clock control inside the sensor subdev allows
> to better match the hardware power on/off sequence requirements,
> however the S5K6A3 sensor can have active clock signal on its clock
> input pin even when all its power supplies are turned off.
>
> I'm posting this series before having a proper implementation for
> clk_unregister() in the clock framework, so you are not blocked with
> your Exynos5 FIMC-IS works.
>


Thank you for the patches. I am modifying exynos5-is based on
the same design.

Thanks & Regards
Arun

Re: [REVIEW PATCH 4/6] exynos4-is: Add clock provider for the external clocks

[Arun Kumar K]

Hi Sylwester,

On Wed, Jul 24, 2013 at 12:09 AM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> This patch adds clock provider to expose the sclk_cam0/1 clocks
> for image sensor subdevs.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../devicetree/bindings/media/samsung-fimc.txt     |   17 +++-
>  drivers/media/platform/exynos4-is/media-dev.c      |   92 ++++++++++++++++++++
>  drivers/media/platform/exynos4-is/media-dev.h      |   19 +++-
>  3 files changed, 125 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt
> index 96312f6..04a2b87 100644
> --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
> +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
> @@ -91,6 +91,15 @@ Optional properties
>  - samsung,camclk-out : specifies clock output for remote sensor,
>                        0 - CAM_A_CLKOUT, 1 - CAM_B_CLKOUT;
>
> +'clock-controller' node (optional)
> +----------------------------------
> +
> +The purpose of this node is to define a clock provider for external image
> +sensors and link any of the CAM_?_CLKOUT clock outputs with related external
> +clock consumer device. Properties specific to this node are described in
> +../clock/clock-bindings.txt.
> +
> +
>  Image sensor nodes
>  ------------------
>
> @@ -114,7 +123,7 @@ Example:
>                         vddio-supply = <...>;
>
>                         clock-frequency = <24000000>;
> -                       clocks = <...>;
> +                       clocks = <&camclk 1>;
>                         clock-names = "mclk";
>
>                         port {
> @@ -135,7 +144,7 @@ Example:
>                         vddio-supply = <...>;
>
>                         clock-frequency = <24000000>;
> -                       clocks = <...>;
> +                       clocks = <&camclk 0>;
>                         clock-names = "mclk";
>
>                         port {
> @@ -156,6 +165,10 @@ Example:
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&cam_port_a_clk_active>;
>
> +               camclk: clock-controller {
> +                      #clock-cells = 1;

Isn't it
                          #clock-cells = <1>;
?

> +               };
> +


Regards
Arun

Re: [REVIEW PATCH 4/6] exynos4-is: Add clock provider for the external clocks

[Sylwester Nawrocki]

Hi Arun,

On 07/29/2013 07:52 AM, Arun Kumar K wrote:
> Hi Sylwester,
> 
> On Wed, Jul 24, 2013 at 12:09 AM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> This patch adds clock provider to expose the sclk_cam0/1 clocks
>> for image sensor subdevs.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  .../devicetree/bindings/media/samsung-fimc.txt     |   17 +++-
>>  drivers/media/platform/exynos4-is/media-dev.c      |   92 ++++++++++++++++++++
>>  drivers/media/platform/exynos4-is/media-dev.h      |   19 +++-
>>  3 files changed, 125 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> index 96312f6..04a2b87 100644
>> --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> @@ -91,6 +91,15 @@ Optional properties
>>  - samsung,camclk-out : specifies clock output for remote sensor,
>>                        0 - CAM_A_CLKOUT, 1 - CAM_B_CLKOUT;
>>
>> +'clock-controller' node (optional)
>> +----------------------------------
>> +
>> +The purpose of this node is to define a clock provider for external image
>> +sensors and link any of the CAM_?_CLKOUT clock outputs with related external
>> +clock consumer device. Properties specific to this node are described in
>> +../clock/clock-bindings.txt.
>> +
>> +
>>  Image sensor nodes
>>  ------------------
>>
>> @@ -114,7 +123,7 @@ Example:
>>                         vddio-supply = <...>;
>>
>>                         clock-frequency = <24000000>;
>> -                       clocks = <...>;
>> +                       clocks = <&camclk 1>;
>>                         clock-names = "mclk";
>>
>>                         port {
>> @@ -135,7 +144,7 @@ Example:
>>                         vddio-supply = <...>;
>>
>>                         clock-frequency = <24000000>;
>> -                       clocks = <...>;
>> +                       clocks = <&camclk 0>;
>>                         clock-names = "mclk";
>>
>>                         port {
>> @@ -156,6 +165,10 @@ Example:
>>                 pinctrl-names = "default";
>>                 pinctrl-0 = <&cam_port_a_clk_active>;
>>
>> +               camclk: clock-controller {
>> +                      #clock-cells = 1;
> 
> Isn't it
>                           #clock-cells = <1>;
> ?

Yes, indeed. Thanks for spotting this!

--
Regards,
Sylwester