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

Message ID 1396216471-11532-3-git-send-email-ben.dooks@codethink.co.uk (mailing list archive)
State RFC, archived
Delegated to: Guennadi Liakhovetski
Headers

Commit Message

Ben Dooks March 30, 2014, 9:54 p.m. UTC
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(-)
  

Comments

Josh Wu March 31, 2014, 9:28 a.m. UTC | #1
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]
--
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
  
Ben Dooks March 31, 2014, 10:10 a.m. UTC | #2
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]
  
Josh Wu April 1, 2014, 6:39 a.m. UTC | #3
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
--
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
  
Ben Dooks April 1, 2014, 1:15 p.m. UTC | #4
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.
  

Patch

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