diff mbox

[4/5] exynos4-is: Remove requirement for "simple-bus" compatible

Message ID 1397583272-28295-5-git-send-email-s.nawrocki@samsung.com
State Superseded, archived
Headers show

Commit Message

Sylwester Nawrocki April 15, 2014, 5:34 p.m. UTC 
This patch makes the driver instantiating its child devices itself,
rather than relying on an OS to instantiate devices as children
of "simple-bus". This removes an incorrect usage of "simple-bus"
compatible.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
This patch addresses Mark's review comments:
https://patchwork.linuxtv.org/patch/23008/
---
 .../devicetree/bindings/media/samsung-fimc.txt     |    6 +--
 drivers/media/platform/exynos4-is/common.c         |   47 +++++++++++++++++++-
 drivers/media/platform/exynos4-is/common.h         |    6 ++-
 drivers/media/platform/exynos4-is/fimc-is.c        |   12 ++++-
 drivers/media/platform/exynos4-is/media-dev.c      |   33 ++++++++++----
 5 files changed, 89 insertions(+), 15 deletions(-)

Comments

Rob Herring April 16, 2014, 3:34 p.m. UTC | #1 
On Tue, Apr 15, 2014 at 12:34 PM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> This patch makes the driver instantiating its child devices itself,
> rather than relying on an OS to instantiate devices as children
> of "simple-bus". This removes an incorrect usage of "simple-bus"
> compatible.

Good, but why can't you use of_platform_populate with the root being
the "samsung,fimc" node? The code to instantiate the devices belongs
in the core OF code.

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
Sylwester Nawrocki April 17, 2014, 9:29 a.m. UTC | #2 
(restoring the Cc list I cleared accidentally in previous reply)

On 16/04/14 21:29, Rob Herring wrote:
> On Wed, Apr 16, 2014 at 12:19 PM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> On 16/04/14 17:34, Rob Herring wrote:
>>> On Tue, Apr 15, 2014 at 12:34 PM, Sylwester Nawrocki
>>> <s.nawrocki@samsung.com> wrote:
>>>>> This patch makes the driver instantiating its child devices itself,
>>>>> rather than relying on an OS to instantiate devices as children
>>>>> of "simple-bus". This removes an incorrect usage of "simple-bus"
>>>>> compatible.
>>>
>>> Good, but why can't you use of_platform_populate with the root being
>>> the "samsung,fimc" node? The code to instantiate the devices belongs
>>> in the core OF code.
>>
>> As I mentioned in other thread, I couldn't see anything like
>> of_platform_unpopulate(), which would allow to destroy any created
>> devices. I can't have of_platform_populate() called as last thing
>> in probe() as some drivers do, so at least deferred probe works.
>> Anyway, it wouldn't be a solution since on a driver removal the
>> created devices must be unregistered.
> 
> I think the deferred probe will get fixed in 3.16, but I'm not
> following how deferred probe is relevant here.

What I meant was that when something fails in the middle of probe() 
callback and of_platform_populate() was already called any devices 
created by it must be destroyed before returning an error from probe().
And some drivers seem to never free their devices created by 
of_platform_populate().

>> I read through thread [1] and I didn't immediately have an idea how
>> to fix the core OF code. So I thought I'd come up with this partial
>> solution.
>>
>> I was wondering if creating functions like of_platform_device_delete(),
>> of_amba_device_delete() and a function that would walk device tree and
>> call them would be a way to go ? I could spend some time on that, any
>> suggestions would be appreciated.
> 
> I need to look at the other removal case, but perhaps the way you did
> using children of the struct device parent is the right way. I'm fine
> with that, but I just want to see this in the core code.

All right, I'll have a closer look then if it can be done that way.
 
> Rob
> 
>>
>> --
>> Regards,
>> Sylwester
>>
>> [1] http://www.spinics.net/lists/linux-omap/msg94484.html

--
Thanks,
Sylwester
--
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/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt
index 922d6f8..8ce5984 100644
--- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
+++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
@@ -15,9 +15,9 @@  Common 'camera' node
 
 Required properties:
 
-- compatible: must be "samsung,fimc", "simple-bus"
+- compatible: should be "samsung,fimc",
 - clocks: list of clock specifiers, corresponding to entries in
-  the clock-names property;
+  the clock-names property,
 - clock-names : must contain "sclk_cam0", "sclk_cam1", "pxl_async0",
   "pxl_async1" entries, matching entries in the clocks property.
 
@@ -157,7 +157,7 @@  Example:
 	};
 
 	camera {
-		compatible = "samsung,fimc", "simple-bus";
+		compatible = "samsung,fimc";
 		clocks = <&clock 132>, <&clock 133>, <&clock 351>,
 			 <&clock 352>;
 		clock-names = "sclk_cam0", "sclk_cam1", "pxl_async0",
diff --git a/drivers/media/platform/exynos4-is/common.c b/drivers/media/platform/exynos4-is/common.c
index 0eb34ec..1e50a48 100644
--- a/drivers/media/platform/exynos4-is/common.c
+++ b/drivers/media/platform/exynos4-is/common.c
@@ -1,7 +1,7 @@ 
 /*
  * Samsung S5P/EXYNOS4 SoC Camera Subsystem driver
  *
- * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Copyright (C) 2013 - 2014 Samsung Electronics Co., Ltd.
  * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
  *
  * This program is free software; you can redistribute it and/or modify
@@ -11,6 +11,7 @@ 
 
 #include <linux/module.h>
 #include <media/exynos-fimc.h>
+#include <linux/of_device.h>
 #include "common.h"
 
 /* Called with the media graph mutex held or entity->stream_count > 0. */
@@ -50,4 +51,48 @@  void __fimc_vidioc_querycap(struct device *dev, struct v4l2_capability *cap,
 }
 EXPORT_SYMBOL(__fimc_vidioc_querycap);
 
+static int __remove_child_device(struct device *dev, void *unused)
+{
+        of_device_unregister(to_platform_device(dev));
+        return 0;
+}
+
+void fimc_unregister_child_devices(struct device *parent)
+{
+        device_for_each_child(parent, NULL, __remove_child_device);
+}
+EXPORT_SYMBOL(fimc_unregister_child_devices);
+
+struct platform_device *fimc_register_child_device(struct device_node *child,
+						   struct device *parent)
+{
+	struct platform_device *pdev;
+
+	pdev = of_platform_device_create(child, NULL, parent);
+	if (!pdev)
+		dev_err(parent,
+			"failed to create device for node %s\n",
+			child->full_name);
+	return pdev;
+}
+EXPORT_SYMBOL(fimc_register_child_device);
+
+int fimc_register_child_devices(struct device *parent)
+{
+	struct platform_device *pdev;
+	struct device_node *child;
+
+	for_each_available_child_of_node(parent->of_node, child) {
+		pdev = fimc_register_child_device(child, parent);
+		if (!pdev) {
+			of_node_put(child);
+			fimc_unregister_child_devices(parent);
+			return -ENXIO;
+		}
+
+	}
+	return 0;
+}
+EXPORT_SYMBOL(fimc_register_child_devices);
+
 MODULE_LICENSE("GPL");
diff --git a/drivers/media/platform/exynos4-is/common.h b/drivers/media/platform/exynos4-is/common.h
index 75b9c71..1d06715 100644
--- a/drivers/media/platform/exynos4-is/common.h
+++ b/drivers/media/platform/exynos4-is/common.h
@@ -1,5 +1,5 @@ 
 /*
- * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Copyright (C) 2013 - 2014 Samsung Electronics Co., Ltd.
  *
  * 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
@@ -14,3 +14,7 @@ 
 struct v4l2_subdev *fimc_find_remote_sensor(struct media_entity *entity);
 void __fimc_vidioc_querycap(struct device *dev, struct v4l2_capability *cap,
 			    unsigned int caps);
+struct platform_device *fimc_register_child_device(struct device_node *child,
+						   struct device *parent);
+int fimc_register_child_devices(struct device *parent);
+void fimc_unregister_child_devices(struct device *parent);
diff --git a/drivers/media/platform/exynos4-is/fimc-is.c b/drivers/media/platform/exynos4-is/fimc-is.c
index a875090..05eab0f 100644
--- a/drivers/media/platform/exynos4-is/fimc-is.c
+++ b/drivers/media/platform/exynos4-is/fimc-is.c
@@ -33,6 +33,7 @@ 
 #include <linux/videodev2.h>
 #include <media/videobuf2-dma-contig.h>
 
+#include "common.h"
 #include "media-dev.h"
 #include "fimc-is.h"
 #include "fimc-is-command.h"
@@ -201,6 +202,13 @@  static int fimc_is_register_subdevs(struct fimc_is *is)
 	struct device_node *i2c_bus, *child;
 	int ret, index = 0;
 
+	/* Create child devices (FIMC-LITE, I2C...) */
+	if (!of_device_is_compatible(is->pdev->dev.of_node, "simple-bus")) {
+		ret = fimc_register_child_devices(&is->pdev->dev);
+		if (ret < 0)
+			return ret;
+	}
+
 	ret = fimc_isp_subdev_create(&is->isp);
 	if (ret < 0)
 		return ret;
@@ -222,10 +230,10 @@  static int fimc_is_register_subdevs(struct fimc_is *is)
 	return 0;
 }
 
-static int fimc_is_unregister_subdevs(struct fimc_is *is)
+static void fimc_is_unregister_subdevs(struct fimc_is *is)
 {
 	fimc_isp_subdev_destroy(&is->isp);
-	return 0;
+	fimc_unregister_child_devices(&is->pdev->dev);
 }
 
 static int fimc_is_load_setfile(struct fimc_is *is, char *file_name)
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index 0b64ec8..975063c 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -33,6 +33,7 @@ 
 #include <media/media-device.h>
 #include <media/exynos-fimc.h>
 
+#include "common.h"
 #include "media-dev.h"
 #include "fimc-core.h"
 #include "fimc-is.h"
@@ -625,19 +626,28 @@  dev_unlock:
 }
 
 /* Register FIMC, FIMC-LITE and CSIS media entities */
-static int fimc_md_register_platform_entities(struct fimc_md *fmd,
-					      struct device_node *parent)
+static int fimc_md_register_platform_entities(struct fimc_md *fmd)
 {
+	struct device *parent = &fmd->pdev->dev;
+	bool create_devices = !of_device_is_compatible(parent->of_node,
+						       "simple-bus");
 	struct device_node *node;
 	int ret = 0;
 
-	for_each_available_child_of_node(parent, node) {
+	for_each_available_child_of_node(parent->of_node, node) {
 		struct platform_device *pdev;
 		int plat_entity = -1;
 
-		pdev = of_find_device_by_node(node);
-		if (!pdev)
-			continue;
+		if (create_devices) {
+			pdev = fimc_register_child_device(node, parent);
+			if (!pdev)
+				return -ENXIO;
+		} else {
+			/* For backward compatibility with old dtb only */
+			pdev = of_find_device_by_node(node);
+			if (!pdev)
+				continue;
+		}
 
 		/* If driver of any entity isn't ready try all again later. */
 		if (!strcmp(node->name, CSIS_OF_NODE_NAME))
@@ -653,7 +663,9 @@  static int fimc_md_register_platform_entities(struct fimc_md *fmd,
 		if (plat_entity >= 0)
 			ret = fimc_md_register_platform_entity(fmd, pdev,
 							plat_entity);
-		put_device(&pdev->dev);
+		if (!create_devices)
+			put_device(&pdev->dev);
+
 		if (ret < 0)
 			break;
 	}
@@ -690,6 +702,11 @@  static void fimc_md_unregister_entities(struct fimc_md *fmd)
 
 	if (fmd->fimc_is)
 		v4l2_device_unregister_subdev(&fmd->fimc_is->isp.subdev);
+	/*
+	 * Free any registered devices corresponding to the camera node
+	 * child nodes.
+	 */
+	fimc_unregister_child_devices(&fmd->pdev->dev);
 
 	v4l2_info(&fmd->v4l2_dev, "Unregistered all entities\n");
 }
@@ -1374,7 +1391,7 @@  static int fimc_md_probe(struct platform_device *pdev)
 	/* Protect the media graph while we're registering entities */
 	mutex_lock(&fmd->media_dev.graph_mutex);
 
-	ret = fimc_md_register_platform_entities(fmd, dev->of_node);
+	ret = fimc_md_register_platform_entities(fmd);
 	if (ret) {
 		mutex_unlock(&fmd->media_dev.graph_mutex);
 		goto err_clk;