[4/5] exynos4-is: Remove requirement for "simple-bus" compatible
Commit Message
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
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 linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
(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 linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
@@ -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",
@@ -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");
@@ -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);
@@ -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)
@@ -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;