[RFCv3 1/3] rcar_vin: copy flags from pdata

[RFCv3 1/3] rcar_vin: copy flags from pdata

[Ben Dooks]

The platform data is a single word, so simply copy
it into the device's private data structure than
keeping a copy of the pointer.

This will make changing to device-tree binding
easier as it is one allocation instead of two.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/media/platform/soc_camera/rcar_vin.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 702dc47..47516df 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -126,13 +126,13 @@ struct rcar_vin_priv {
 	int				sequence;
 	/* State of the VIN module in capturing mode */
 	enum rcar_vin_state		state;
-	struct rcar_vin_platform_data	*pdata;
 	struct soc_camera_host		ici;
 	struct list_head		capture;
 #define MAX_BUFFER_NUM			3
 	struct vb2_buffer		*queue_buf[MAX_BUFFER_NUM];
 	struct vb2_alloc_ctx		*alloc_ctx;
 	enum v4l2_field			field;
+	unsigned int			pdata_flags;
 	unsigned int			vb_count;
 	unsigned int			nr_hw_slots;
 	bool				request_to_stop;
@@ -275,12 +275,12 @@ static int rcar_vin_setup(struct rcar_vin_priv *priv)
 		break;
 	case V4L2_MBUS_FMT_YUYV8_2X8:
 		/* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
-		vnmc |= priv->pdata->flags & RCAR_VIN_BT656 ?
+		vnmc |= priv->pdata_flags & RCAR_VIN_BT656 ?
 			VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601;
 		break;
 	case V4L2_MBUS_FMT_YUYV10_2X10:
 		/* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */
-		vnmc |= priv->pdata->flags & RCAR_VIN_BT656 ?
+		vnmc |= priv->pdata_flags & RCAR_VIN_BT656 ?
 			VNMC_INF_YUV10_BT656 : VNMC_INF_YUV10_BT601;
 		break;
 	default:
@@ -799,7 +799,7 @@ static int rcar_vin_set_bus_param(struct soc_camera_device *icd)
 	/* Make choises, based on platform preferences */
 	if ((common_flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) &&
 	    (common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)) {
-		if (priv->pdata->flags & RCAR_VIN_HSYNC_ACTIVE_LOW)
+		if (priv->pdata_flags & RCAR_VIN_HSYNC_ACTIVE_LOW)
 			common_flags &= ~V4L2_MBUS_HSYNC_ACTIVE_HIGH;
 		else
 			common_flags &= ~V4L2_MBUS_HSYNC_ACTIVE_LOW;
@@ -807,7 +807,7 @@ static int rcar_vin_set_bus_param(struct soc_camera_device *icd)
 
 	if ((common_flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) &&
 	    (common_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)) {
-		if (priv->pdata->flags & RCAR_VIN_VSYNC_ACTIVE_LOW)
+		if (priv->pdata_flags & RCAR_VIN_VSYNC_ACTIVE_LOW)
 			common_flags &= ~V4L2_MBUS_VSYNC_ACTIVE_HIGH;
 		else
 			common_flags &= ~V4L2_MBUS_VSYNC_ACTIVE_LOW;
@@ -1447,7 +1447,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
 	priv->ici.drv_name = dev_name(&pdev->dev);
 	priv->ici.ops = &rcar_vin_host_ops;
 
-	priv->pdata = pdata;
+	priv->pdata_flags = pdata->flags;
 	priv->chip = pdev->id_entry->driver_data;
 	spin_lock_init(&priv->lock);
 	INIT_LIST_HEAD(&priv->capture);
-- 
1.9.0

[RFCv3 2/3] rcar_vin: add devicetree support

[Ben Dooks]

Add support for devicetree probe for the rcar-vin
driver.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
Changes since v1:
	- fix pdev->id handling via usage of alias property
Changes since v2:
	- fix documentation for v1 updates
---
 .../devicetree/bindings/media/rcar_vin.txt         | 86 ++++++++++++++++++++++
 drivers/media/platform/soc_camera/rcar_vin.c       | 70 ++++++++++++++++--
 2 files changed, 149 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/rcar_vin.txt

diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
new file mode 100644
index 0000000..5463615
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
@@ -0,0 +1,86 @@
+Renesas RCar Video Input driver (rcar_vin)
+------------------------------------------
+
+The rcar_vin device provides video input capabilities for the Renesas R-Car
+family of devices. The current blocks are always slaves and suppot one input
+channel which can be either RGB, YUYV or BT656.
+
+ - compatible: Must be one of the following
+   - "renesas,vin-r8a7791" for the R8A7791 device
+   - "renesas,vin-r8a7790" for the R8A7790 device
+   - "renesas,vin-r8a7779" for the R8A7779 device
+   - "renesas,vin-r8a7778" for the R8A7778 device
+ - reg: the register base and size for the device registers
+ - interrupts: the interrupt for the device
+ - clocks: Reference to the parent clock
+
+Additionally, an alias named vinX will need to be created to specify
+which video input device this is.
+
+The per-board settings:
+ - port sub-node describing a single endpoint connected to the vin
+   as described in video-interfaces.txt[1]. Only the first one will
+   be considered as each vin interface has one input port.
+
+   These settings are used to work out video input format and widths
+   into the system.
+
+
+Device node example
+-------------------
+
+	aliases {
+	       vin0 = &vin0;
+	};
+
+        vin0: vin@0xe6ef0000 {
+                compatible = "renesas,vin-r8a7790";
+                clocks = <&mstp8_clks R8A7790_CLK_VIN0>;
+                reg = <0 0xe6ef0000 0 0x1000>;
+                interrupts = <0 188 IRQ_TYPE_LEVEL_HIGH>;
+                status = "disabled";
+        };
+
+Board setup example (vin1 composite video input)
+------------------------------------------------
+
+&i2c2   {
+        status = "ok";
+        pinctrl-0 = <&i2c2_pins>;
+        pinctrl-names = "default";
+
+        adv7180: adv7180@0x20 {
+                compatible = "adi,adv7180";
+                reg = <0x20>;
+                remote = <&vin1>;
+
+                port {
+                        adv7180_1: endpoint {
+                                bus-width = <8>;
+                                remote-endpoint = <&vin1ep0>;
+                        };
+                };
+        };
+};
+
+/* composite video input */
+&vin1 {
+        pinctrl-0 = <&vin1_pins>;
+        pinctrl-names = "default";
+
+        status = "ok";
+
+        port {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                vin1ep0: endpoint {
+                        remote-endpoint = <&adv7180_1>;
+                        bus-width = <8>;
+                };
+        };
+};
+
+
+
+[1] video-interfaces.txt common video media interface
diff --git a/drivers/media/platform/soc_camera/rcar_vin.c b/drivers/media/platform/soc_camera/rcar_vin.c
index 47516df..72d2504 100644
--- a/drivers/media/platform/soc_camera/rcar_vin.c
+++ b/drivers/media/platform/soc_camera/rcar_vin.c
@@ -24,6 +24,8 @@
 #include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/videodev2.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 #include <media/soc_camera.h>
 #include <media/soc_mediabus.h>
@@ -32,6 +34,7 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-mediabus.h>
 #include <media/v4l2-subdev.h>
+#include <media/v4l2-of.h>
 #include <media/videobuf2-dma-contig.h>
 
 #include "soc_scale_crop.h"
@@ -1392,6 +1395,17 @@ static struct soc_camera_host_ops rcar_vin_host_ops = {
 	.init_videobuf2	= rcar_vin_init_videobuf2,
 };
 
+#ifdef CONFIG_OF
+static struct of_device_id rcar_vin_of_table[] = {
+	{ .compatible = "renesas,vin-r8a7791", .data = (void *)RCAR_GEN2 },
+	{ .compatible = "renesas,vin-r8a7790", .data = (void *)RCAR_GEN2 },
+	{ .compatible = "renesas,vin-r8a7779", .data = (void *)RCAR_H1 },
+	{ .compatible = "renesas,vin-r8a7778", .data = (void *)RCAR_M1 },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, rcar_vin_of_table);
+#endif
+
 static struct platform_device_id rcar_vin_id_table[] = {
 	{ "r8a7791-vin",  RCAR_GEN2 },
 	{ "r8a7790-vin",  RCAR_GEN2 },
@@ -1404,15 +1418,50 @@ MODULE_DEVICE_TABLE(platform, rcar_vin_id_table);
 
 static int rcar_vin_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *match = NULL;
 	struct rcar_vin_priv *priv;
 	struct resource *mem;
 	struct rcar_vin_platform_data *pdata;
+	unsigned int pdata_flags;
 	int irq, ret;
 
-	pdata = pdev->dev.platform_data;
-	if (!pdata || !pdata->flags) {
-		dev_err(&pdev->dev, "platform data not set\n");
-		return -EINVAL;
+	if (pdev->dev.of_node) {
+		struct v4l2_of_endpoint ep;
+		struct device_node *np;
+
+		match = of_match_device(of_match_ptr(rcar_vin_of_table),
+					&pdev->dev);
+
+		np = v4l2_of_get_next_endpoint(pdev->dev.of_node, NULL);
+		if (!np) {
+			dev_err(&pdev->dev, "could not find endpoint\n");
+			return -EINVAL;
+		}
+
+		ret = v4l2_of_parse_endpoint(np, &ep);
+		if (ret) {
+			dev_err(&pdev->dev, "could not parse endpoint\n");
+			return ret;
+		}
+
+		if (ep.bus_type == V4L2_MBUS_BT656)
+			pdata_flags = RCAR_VIN_BT656;
+		else {
+			pdata_flags = 0;
+			if (ep.bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
+				pdata_flags |= RCAR_VIN_HSYNC_ACTIVE_LOW;
+			if (ep.bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
+				pdata_flags |= RCAR_VIN_VSYNC_ACTIVE_LOW;
+		}
+
+		dev_dbg(&pdev->dev, "pdata_flags = %08x\n", pdata_flags);
+	} else {
+		pdata = pdev->dev.platform_data;
+		if (!pdata || !pdata->flags) {
+			dev_err(&pdev->dev, "platform data not set\n");
+			return -EINVAL;
+		}
+		pdata_flags = pdata->flags;
 	}
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -1443,12 +1492,18 @@ static int rcar_vin_probe(struct platform_device *pdev)
 
 	priv->ici.priv = priv;
 	priv->ici.v4l2_dev.dev = &pdev->dev;
-	priv->ici.nr = pdev->id;
 	priv->ici.drv_name = dev_name(&pdev->dev);
 	priv->ici.ops = &rcar_vin_host_ops;
 
-	priv->pdata_flags = pdata->flags;
-	priv->chip = pdev->id_entry->driver_data;
+	priv->pdata_flags = pdata_flags;
+	if (!match) {
+		priv->ici.nr = pdev->id;
+		priv->chip = pdev->id_entry->driver_data;
+	} else {
+		priv->ici.nr = of_alias_get_id(pdev->dev.of_node, "vin");
+		priv->chip = (enum chip_id)match->data;
+	};
+
 	spin_lock_init(&priv->lock);
 	INIT_LIST_HEAD(&priv->capture);
 
@@ -1489,6 +1544,7 @@ static struct platform_driver rcar_vin_driver = {
 	.driver		= {
 		.name		= DRV_NAME,
 		.owner		= THIS_MODULE,
+		.of_match_table	= of_match_ptr(rcar_vin_of_table),
 	},
 	.id_table	= rcar_vin_id_table,
 };
-- 
1.9.0

[RFCv3 3/3] soc_camera: initial of code

[Ben Dooks]

Add initial support for OF based soc-camera devices that may be used
by any of the soc-camera drivers. The driver itself will need converting
to use OF.

These changes allow the soc-camera driver to do the connecting of any
async capable v4l2 device to the soc-camera driver. This has currently
been tested on the Renesas Lager board.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/media/platform/soc_camera/soc_camera.c | 111 ++++++++++++++++++++++++-
 1 file changed, 110 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 4b8c024..afe22d4 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -36,6 +36,7 @@
 #include <media/v4l2-common.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-dev.h>
+#include <media/v4l2-of.h>
 #include <media/videobuf-core.h>
 #include <media/videobuf2-core.h>
 
@@ -1579,6 +1580,112 @@ static void scan_async_host(struct soc_camera_host *ici)
 #define scan_async_host(ici)		do {} while (0)
 #endif
 
+#ifdef CONFIG_OF
+static int soc_of_bind(struct soc_camera_host *ici,
+		       struct device_node *ep,
+		       struct device_node *remote)
+{
+	struct soc_camera_device *icd;
+	struct soc_camera_desc sdesc = {.host_desc.bus_id = ici->nr,};
+	struct soc_camera_async_client *sasc;
+	struct soc_camera_async_subdev *sasd;
+	struct v4l2_async_subdev **asd_array;
+	char clk_name[V4L2_SUBDEV_NAME_SIZE];
+	int ret;
+
+	/* alloacte a new subdev and add match info to it */
+	sasd = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasd), GFP_KERNEL);
+	if (!sasd)
+		return -ENOMEM;
+
+	asd_array = devm_kzalloc(ici->v4l2_dev.dev,
+				 sizeof(struct v4l2_async_subdev **),
+				 GFP_KERNEL);
+	if (!asd_array)
+		return -ENOMEM;
+
+	sasd->asd.match.of.node = remote;
+	sasd->asd.match_type = V4L2_ASYNC_MATCH_OF;
+	asd_array[0] = &sasd->asd;
+
+	/* Or shall this be managed by the soc-camera device? */
+	sasc = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasc), GFP_KERNEL);
+	if (!sasc)
+		return -ENOMEM;
+
+	/* HACK: just need a != NULL */
+	sdesc.host_desc.board_info = ERR_PTR(-ENODATA);
+
+	ret = soc_camera_dyn_pdev(&sdesc, sasc);
+	if (ret < 0)
+		return ret;
+
+	sasc->sensor = &sasd->asd;
+
+	icd = soc_camera_add_pdev(sasc);
+	if (!icd) {
+		platform_device_put(sasc->pdev);
+		return -ENOMEM;
+	}
+
+	//sasc->notifier.subdevs = asd;
+	sasc->notifier.subdevs = asd_array;
+	sasc->notifier.num_subdevs = 1;
+	sasc->notifier.bound = soc_camera_async_bound;
+	sasc->notifier.unbind = soc_camera_async_unbind;
+	sasc->notifier.complete = soc_camera_async_complete;
+
+	icd->sasc = sasc;
+	icd->parent = ici->v4l2_dev.dev;
+
+	snprintf(clk_name, sizeof(clk_name), "of-%s",
+		 of_node_full_name(remote));
+
+	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd);
+	if (IS_ERR(icd->clk)) {
+		ret = PTR_ERR(icd->clk);
+		goto eclkreg;
+	}
+
+	ret = v4l2_async_notifier_register(&ici->v4l2_dev, &sasc->notifier);
+	if (!ret)
+		return 0;
+
+eclkreg:
+	icd->clk = NULL;
+	platform_device_unregister(sasc->pdev);
+	dev_err(ici->v4l2_dev.dev, "group probe failed: %d\n", ret);
+
+	return ret;
+}
+
+static inline void scan_of_host(struct soc_camera_host *ici)
+{
+	struct device_node *np = ici->v4l2_dev.dev->of_node;
+	struct device_node *epn = NULL;
+	struct device_node *ren;
+
+	while (true) {
+		epn = v4l2_of_get_next_endpoint(np, epn);
+		if (!epn)
+			break;
+
+		ren = v4l2_of_get_remote_port(epn);
+		if (!ren) {
+			pr_info("%s: no remote for %s\n",
+				__func__,  of_node_full_name(epn));
+			continue;
+		}
+
+		/* so we now have a remote node to connect */
+		soc_of_bind(ici, epn, ren->parent);
+	}
+}
+
+#else
+static inline void scan_of_host(struct soc_camera_host *ici) { }
+#endif
+
 /* Called during host-driver probe */
 static int soc_camera_probe(struct soc_camera_host *ici,
 			    struct soc_camera_device *icd)
@@ -1830,7 +1937,9 @@ int soc_camera_host_register(struct soc_camera_host *ici)
 	mutex_init(&ici->host_lock);
 	mutex_init(&ici->clk_lock);
 
-	if (ici->asd_sizes)
+	if (ici->v4l2_dev.dev->of_node)
+		scan_of_host(ici);
+	else if (ici->asd_sizes)
 		/*
 		 * No OF, host with a list of subdevices. Don't try to mix
 		 * modes by initialising some groups statically and some
-- 
1.9.0

Re: [RFCv3,3/3] soc_camera: initial of code

[Josh Wu]

Hi, Ben 

Thanks for the patch, I just test atmel-isi with the your patch,
I find the "mclk" registered in soc-camera driver cannot be find
by the soc-camera sensors. See comment in below:

> [snip]

> ... ...

> +#ifdef CONFIG_OF
> +static int soc_of_bind(struct soc_camera_host *ici,
> +		       struct device_node *ep,
> +		       struct device_node *remote)
> +{
> +	struct soc_camera_device *icd;
> +	struct soc_camera_desc sdesc = {.host_desc.bus_id = ici->nr,};
> +	struct soc_camera_async_client *sasc;
> +	struct soc_camera_async_subdev *sasd;
> +	struct v4l2_async_subdev **asd_array;
> +	char clk_name[V4L2_SUBDEV_NAME_SIZE];
> +	int ret;
> +
> +	/* alloacte a new subdev and add match info to it */
> +	sasd = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasd), GFP_KERNEL);
> +	if (!sasd)
> +		return -ENOMEM;
> +
> +	asd_array = devm_kzalloc(ici->v4l2_dev.dev,
> +				 sizeof(struct v4l2_async_subdev **),
> +				 GFP_KERNEL);
> +	if (!asd_array)
> +		return -ENOMEM;
> +
> +	sasd->asd.match.of.node = remote;
> +	sasd->asd.match_type = V4L2_ASYNC_MATCH_OF;
> +	asd_array[0] = &sasd->asd;
> +
> +	/* Or shall this be managed by the soc-camera device? */
> +	sasc = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasc), GFP_KERNEL);
> +	if (!sasc)
> +		return -ENOMEM;
> +
> +	/* HACK: just need a != NULL */
> +	sdesc.host_desc.board_info = ERR_PTR(-ENODATA);
> +
> +	ret = soc_camera_dyn_pdev(&sdesc, sasc);
> +	if (ret < 0)
> +		return ret;
> +
> +	sasc->sensor = &sasd->asd;
> +
> +	icd = soc_camera_add_pdev(sasc);
> +	if (!icd) {
> +		platform_device_put(sasc->pdev);
> +		return -ENOMEM;
> +	}
> +
> +	//sasc->notifier.subdevs = asd;
> +	sasc->notifier.subdevs = asd_array;
> +	sasc->notifier.num_subdevs = 1;
> +	sasc->notifier.bound = soc_camera_async_bound;
> +	sasc->notifier.unbind = soc_camera_async_unbind;
> +	sasc->notifier.complete = soc_camera_async_complete;
> +
> +	icd->sasc = sasc;
> +	icd->parent = ici->v4l2_dev.dev;
> +
> +	snprintf(clk_name, sizeof(clk_name), "of-%s",
> +		 of_node_full_name(remote));

The clk_name you register here is a OF string, but for the i2c sensors, which
call the v4l2_clk_get by using dev_name(&client->dev). It is format like:
"1-0030", combined i2c adaptor ID and addr.
So the i2c sensor can not find this registered mclk as the name is not match.

Best Regards,
Josh Wu

> +
> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd);
> +	if (IS_ERR(icd->clk)) {
> +		ret = PTR_ERR(icd->clk);
> +		goto eclkreg;
> +	}
> +


> [snip]

Re: [RFCv3,3/3] soc_camera: initial of code

[Ben Dooks]

On 31/03/14 10:28, Josh Wu wrote:
> Hi, Ben
>
> Thanks for the patch, I just test atmel-isi with the your patch,
> I find the "mclk" registered in soc-camera driver cannot be find
> by the soc-camera sensors. See comment in below:

Ok, I guess that the driver I have does not need the
mclk clock.

>
>> ... ...
>
>> +#ifdef CONFIG_OF
>> +static int soc_of_bind(struct soc_camera_host *ici,
>> +		       struct device_node *ep,
>> +		       struct device_node *remote)
>> +{
>> +	struct soc_camera_device *icd;
>> +	struct soc_camera_desc sdesc = {.host_desc.bus_id = ici->nr,};
>> +	struct soc_camera_async_client *sasc;
>> +	struct soc_camera_async_subdev *sasd;
>> +	struct v4l2_async_subdev **asd_array;
>> +	char clk_name[V4L2_SUBDEV_NAME_SIZE];
>> +	int ret;
>> +
>> +	/* alloacte a new subdev and add match info to it */
>> +	sasd = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasd), GFP_KERNEL);
>> +	if (!sasd)
>> +		return -ENOMEM;
>> +
>> +	asd_array = devm_kzalloc(ici->v4l2_dev.dev,
>> +				 sizeof(struct v4l2_async_subdev **),
>> +				 GFP_KERNEL);
>> +	if (!asd_array)
>> +		return -ENOMEM;
>> +
>> +	sasd->asd.match.of.node = remote;
>> +	sasd->asd.match_type = V4L2_ASYNC_MATCH_OF;
>> +	asd_array[0] = &sasd->asd;
>> +
>> +	/* Or shall this be managed by the soc-camera device? */
>> +	sasc = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasc), GFP_KERNEL);
>> +	if (!sasc)
>> +		return -ENOMEM;
>> +
>> +	/* HACK: just need a != NULL */
>> +	sdesc.host_desc.board_info = ERR_PTR(-ENODATA);
>> +
>> +	ret = soc_camera_dyn_pdev(&sdesc, sasc);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	sasc->sensor = &sasd->asd;
>> +
>> +	icd = soc_camera_add_pdev(sasc);
>> +	if (!icd) {
>> +		platform_device_put(sasc->pdev);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	//sasc->notifier.subdevs = asd;
>> +	sasc->notifier.subdevs = asd_array;
>> +	sasc->notifier.num_subdevs = 1;
>> +	sasc->notifier.bound = soc_camera_async_bound;
>> +	sasc->notifier.unbind = soc_camera_async_unbind;
>> +	sasc->notifier.complete = soc_camera_async_complete;
>> +
>> +	icd->sasc = sasc;
>> +	icd->parent = ici->v4l2_dev.dev;
>> +
>> +	snprintf(clk_name, sizeof(clk_name), "of-%s",
>> +		 of_node_full_name(remote));
>
> The clk_name you register here is a OF string, but for the i2c sensors, which
> call the v4l2_clk_get by using dev_name(&client->dev). It is format like:
> "1-0030", combined i2c adaptor ID and addr.
> So the i2c sensor can not find this registered mclk as the name is not match.

Hmm, this sounds like something that really should go
away and be handled by the clk system instead.

[snip]


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [RFCv3,3/3] soc_camera: initial of code

[Josh Wu]

Hi, Ben

On 3/31/2014 6:10 PM, Ben Dooks wrote:
> On 31/03/14 10:28, Josh Wu wrote:
>> Hi, Ben
>>
>> Thanks for the patch, I just test atmel-isi with the your patch,
>> I find the "mclk" registered in soc-camera driver cannot be find
>> by the soc-camera sensors. See comment in below:
>
> Ok, I guess that the driver I have does not need the
> mclk clock.
>
>>
>>> ... ...
>>
>>> +#ifdef CONFIG_OF
>>> +static int soc_of_bind(struct soc_camera_host *ici,
>>> +               struct device_node *ep,
>>> +               struct device_node *remote)
>>> +{
>>> +    struct soc_camera_device *icd;
>>> +    struct soc_camera_desc sdesc = {.host_desc.bus_id = ici->nr,};
>>> +    struct soc_camera_async_client *sasc;
>>> +    struct soc_camera_async_subdev *sasd;
>>> +    struct v4l2_async_subdev **asd_array;
>>> +    char clk_name[V4L2_SUBDEV_NAME_SIZE];
>>> +    int ret;
>>> +
>>> +    /* alloacte a new subdev and add match info to it */
>>> +    sasd = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasd), GFP_KERNEL);
>>> +    if (!sasd)
>>> +        return -ENOMEM;
>>> +
>>> +    asd_array = devm_kzalloc(ici->v4l2_dev.dev,
>>> +                 sizeof(struct v4l2_async_subdev **),
>>> +                 GFP_KERNEL);
>>> +    if (!asd_array)
>>> +        return -ENOMEM;
>>> +
>>> +    sasd->asd.match.of.node = remote;
>>> +    sasd->asd.match_type = V4L2_ASYNC_MATCH_OF;
>>> +    asd_array[0] = &sasd->asd;
>>> +
>>> +    /* Or shall this be managed by the soc-camera device? */
>>> +    sasc = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasc), GFP_KERNEL);
>>> +    if (!sasc)
>>> +        return -ENOMEM;
>>> +
>>> +    /* HACK: just need a != NULL */
>>> +    sdesc.host_desc.board_info = ERR_PTR(-ENODATA);
>>> +
>>> +    ret = soc_camera_dyn_pdev(&sdesc, sasc);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    sasc->sensor = &sasd->asd;
>>> +
>>> +    icd = soc_camera_add_pdev(sasc);
>>> +    if (!icd) {
>>> +        platform_device_put(sasc->pdev);
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    //sasc->notifier.subdevs = asd;
>>> +    sasc->notifier.subdevs = asd_array;
>>> +    sasc->notifier.num_subdevs = 1;
>>> +    sasc->notifier.bound = soc_camera_async_bound;
>>> +    sasc->notifier.unbind = soc_camera_async_unbind;
>>> +    sasc->notifier.complete = soc_camera_async_complete;
>>> +
>>> +    icd->sasc = sasc;
>>> +    icd->parent = ici->v4l2_dev.dev;
>>> +
>>> +    snprintf(clk_name, sizeof(clk_name), "of-%s",
>>> +         of_node_full_name(remote));
>>
>> The clk_name you register here is a OF string, but for the i2c 
>> sensors, which
>> call the v4l2_clk_get by using dev_name(&client->dev). It is format 
>> like:
>> "1-0030", combined i2c adaptor ID and addr.
>> So the i2c sensor can not find this registered mclk as the name is 
>> not match.
>
> Hmm, this sounds like something that really should go
> away and be handled by the clk system instead.

Since the v4l2 clk (mclk) is just a temperory solution and it will be 
removed if all use common clk framework.
(See the commit message of ff5430de70).

So IMHO we can live with this, just simply add the code in soc_of_bind():

+       struct i2c_client *client;
... ...

          client = of_find_i2c_device_by_node(remote);
+       if (!client)
+               goto eclkreg;

-        snprintf(clk_name, sizeof(clk_name), "of-%s",
-               of_node_full_name(remote));
+       snprintf(clk_name, sizeof(clk_name), "%d-%04x",
+                client->adapter->nr, client->addr);

         icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, 
"mclk", icd);


> [snip]
>
>
Best Regards,
Josh Wu

Re: [RFCv3,3/3] soc_camera: initial of code

[Ben Dooks]

On 01/04/14 07:39, Josh Wu wrote:
> Hi, Ben
>
> On 3/31/2014 6:10 PM, Ben Dooks wrote:
>> On 31/03/14 10:28, Josh Wu wrote:
>>> Hi, Ben
>>>
>>> Thanks for the patch, I just test atmel-isi with the your patch,
>>> I find the "mclk" registered in soc-camera driver cannot be find
>>> by the soc-camera sensors. See comment in below:
>>
>> Ok, I guess that the driver I have does not need the
>> mclk clock.
>>
>>>
>>>> ... ...
>>>
>>>> +#ifdef CONFIG_OF
>>>> +static int soc_of_bind(struct soc_camera_host *ici,
>>>> +               struct device_node *ep,
>>>> +               struct device_node *remote)
>>>> +{
>>>> +    struct soc_camera_device *icd;
>>>> +    struct soc_camera_desc sdesc = {.host_desc.bus_id = ici->nr,};
>>>> +    struct soc_camera_async_client *sasc;
>>>> +    struct soc_camera_async_subdev *sasd;
>>>> +    struct v4l2_async_subdev **asd_array;
>>>> +    char clk_name[V4L2_SUBDEV_NAME_SIZE];
>>>> +    int ret;
>>>> +
>>>> +    /* alloacte a new subdev and add match info to it */
>>>> +    sasd = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasd), GFP_KERNEL);
>>>> +    if (!sasd)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    asd_array = devm_kzalloc(ici->v4l2_dev.dev,
>>>> +                 sizeof(struct v4l2_async_subdev **),
>>>> +                 GFP_KERNEL);
>>>> +    if (!asd_array)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    sasd->asd.match.of.node = remote;
>>>> +    sasd->asd.match_type = V4L2_ASYNC_MATCH_OF;
>>>> +    asd_array[0] = &sasd->asd;
>>>> +
>>>> +    /* Or shall this be managed by the soc-camera device? */
>>>> +    sasc = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasc), GFP_KERNEL);
>>>> +    if (!sasc)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    /* HACK: just need a != NULL */
>>>> +    sdesc.host_desc.board_info = ERR_PTR(-ENODATA);
>>>> +
>>>> +    ret = soc_camera_dyn_pdev(&sdesc, sasc);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    sasc->sensor = &sasd->asd;
>>>> +
>>>> +    icd = soc_camera_add_pdev(sasc);
>>>> +    if (!icd) {
>>>> +        platform_device_put(sasc->pdev);
>>>> +        return -ENOMEM;
>>>> +    }
>>>> +
>>>> +    //sasc->notifier.subdevs = asd;
>>>> +    sasc->notifier.subdevs = asd_array;
>>>> +    sasc->notifier.num_subdevs = 1;
>>>> +    sasc->notifier.bound = soc_camera_async_bound;
>>>> +    sasc->notifier.unbind = soc_camera_async_unbind;
>>>> +    sasc->notifier.complete = soc_camera_async_complete;
>>>> +
>>>> +    icd->sasc = sasc;
>>>> +    icd->parent = ici->v4l2_dev.dev;
>>>> +
>>>> +    snprintf(clk_name, sizeof(clk_name), "of-%s",
>>>> +         of_node_full_name(remote));
>>>
>>> The clk_name you register here is a OF string, but for the i2c
>>> sensors, which
>>> call the v4l2_clk_get by using dev_name(&client->dev). It is format
>>> like:
>>> "1-0030", combined i2c adaptor ID and addr.
>>> So the i2c sensor can not find this registered mclk as the name is
>>> not match.
>>
>> Hmm, this sounds like something that really should go
>> away and be handled by the clk system instead.
>
> Since the v4l2 clk (mclk) is just a temperory solution and it will be
> removed if all use common clk framework.
> (See the commit message of ff5430de70).
>
> So IMHO we can live with this, just simply add the code in soc_of_bind():
>
> +       struct i2c_client *client;
> ... ...
>
>           client = of_find_i2c_device_by_node(remote);
> +       if (!client)
> +               goto eclkreg;
>
> -        snprintf(clk_name, sizeof(clk_name), "of-%s",
> -               of_node_full_name(remote));
> +       snprintf(clk_name, sizeof(clk_name), "%d-%04x",
> +                client->adapter->nr, client->addr);
>
>          icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name,
> "mclk", icd);
>
>
>> [snip]

Thanks, I will look into this.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius