[PATCH 0/2] V4L2: add CCF support to v4l2_clk

[PATCH 0/2] V4L2: add CCF support to v4l2_clk

[Guennadi Liakhovetski]

Hi,

This is an attempt to implement CCF support for v4l2_clk to be able to use 
e.g. DT-based clocks. Beware - completely untested! Josh, could you please 
see, whether you can use this as a starting point?

Thanks
Guennadi

[PATCH 1/2] V4L: remove clock name from v4l2_clk API

[Guennadi Liakhovetski]

All uses of the v4l2_clk API so far only register one clock with a fixed
name. This allows us to get rid of it, which also will make CCF and DT
integration easier.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/platform/soc_camera/soc_camera.c |  6 +++---
 drivers/media/usb/em28xx/em28xx-camera.c       |  2 +-
 drivers/media/v4l2-core/v4l2-clk.c             | 24 +++++++++++-------------
 include/media/v4l2-clk.h                       |  7 +++----
 4 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index f4be2a1..ce192b6 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct soc_camera_device *icd,
 	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
 		 shd->i2c_adapter_id, shd->board_info->addr);
 
-	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd);
+	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
 	if (IS_ERR(icd->clk)) {
 		ret = PTR_ERR(icd->clk);
 		goto eclkreg;
@@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host *ici,
 	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
 		 sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address);
 
-	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd);
+	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
 	if (IS_ERR(icd->clk)) {
 		ret = PTR_ERR(icd->clk);
 		goto eclkreg;
@@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
 		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);
+	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
 	if (IS_ERR(icd->clk)) {
 		ret = PTR_ERR(icd->clk);
 		goto eclkreg;
diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
index 7be661f..a4b22c2 100644
--- a/drivers/media/usb/em28xx/em28xx-camera.c
+++ b/drivers/media/usb/em28xx/em28xx-camera.c
@@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev)
 
 	v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
 			  i2c_adapter_id(adap), client->addr);
-	v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL);
+	v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL);
 	if (IS_ERR(v4l2->clk))
 		return PTR_ERR(v4l2->clk);
 
diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
index e18cc04..c210906 100644
--- a/drivers/media/v4l2-core/v4l2-clk.c
+++ b/drivers/media/v4l2-core/v4l2-clk.c
@@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id)
 		if (strcmp(dev_id, clk->dev_id))
 			continue;
 
-		if (!id || !clk->id || !strcmp(clk->id, id))
+		if ((!id && !clk->id) ||
+		    (id && clk->id && !strcmp(clk->id, id)))
 			return clk;
 	}
 
@@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
 	mutex_lock(&clk->lock);
 
 	enable = --clk->enable;
-	if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__,
-		 clk->dev_id, clk->id))
+	if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__,
+		 clk->dev_id))
 		clk->enable++;
 	else if (!enable && clk->ops->disable)
 		clk->ops->disable(clk);
@@ -181,7 +182,7 @@ EXPORT_SYMBOL(v4l2_clk_set_rate);
 
 struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
 				   const char *dev_id,
-				   const char *id, void *priv)
+				   void *priv)
 {
 	struct v4l2_clk *clk;
 	int ret;
@@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
 	if (!clk)
 		return ERR_PTR(-ENOMEM);
 
-	clk->id = kstrdup(id, GFP_KERNEL);
 	clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
-	if ((id && !clk->id) || !clk->dev_id) {
+	if (!clk->dev_id) {
 		ret = -ENOMEM;
 		goto ealloc;
 	}
@@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
 	mutex_init(&clk->lock);
 
 	mutex_lock(&clk_lock);
-	if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
+	if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) {
 		mutex_unlock(&clk_lock);
 		ret = -EEXIST;
 		goto eexist;
@@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
 
 eexist:
 ealloc:
-	kfree(clk->id);
 	kfree(clk->dev_id);
 	kfree(clk);
 	return ERR_PTR(ret);
@@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register);
 void v4l2_clk_unregister(struct v4l2_clk *clk)
 {
 	if (WARN(atomic_read(&clk->use_count),
-		 "%s(): Refusing to unregister ref-counted %s:%s clock!\n",
-		 __func__, clk->dev_id, clk->id))
+		 "%s(): Refusing to unregister ref-counted %s clock!\n",
+		 __func__, clk->dev_id))
 		return;
 
 	mutex_lock(&clk_lock);
 	list_del(&clk->list);
 	mutex_unlock(&clk_lock);
 
-	kfree(clk->id);
 	kfree(clk->dev_id);
 	kfree(clk);
 }
@@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk *clk)
 }
 
 struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
-		const char *id, unsigned long rate, struct module *owner)
+				unsigned long rate, struct module *owner)
 {
 	struct v4l2_clk *clk;
 	struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL);
@@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
 	priv->ops.get_rate = fixed_get_rate;
 	priv->ops.owner = owner;
 
-	clk = v4l2_clk_register(&priv->ops, dev_id, id, priv);
+	clk = v4l2_clk_register(&priv->ops, dev_id, priv);
 	if (IS_ERR(clk))
 		kfree(priv);
 
diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
index 0b36cc1..8f06967 100644
--- a/include/media/v4l2-clk.h
+++ b/include/media/v4l2-clk.h
@@ -43,7 +43,7 @@ struct v4l2_clk_ops {
 
 struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
 				   const char *dev_name,
-				   const char *name, void *priv);
+				   void *priv);
 void v4l2_clk_unregister(struct v4l2_clk *clk);
 struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id);
 void v4l2_clk_put(struct v4l2_clk *clk);
@@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate);
 struct module;
 
 struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
-		const char *id, unsigned long rate, struct module *owner);
+			unsigned long rate, struct module *owner);
 void v4l2_clk_unregister_fixed(struct v4l2_clk *clk);
 
 static inline struct v4l2_clk *v4l2_clk_register_fixed(const char *dev_id,
-							const char *id,
 							unsigned long rate)
 {
-	return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE);
+	return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE);
 }
 
 #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, \
-- 
1.9.3

[PATCH 2/2] V4L2: add CCF support to the v4l2_clk API

[Guennadi Liakhovetski]

V4L2 clocks, e.g. used by camera sensors for their master clock, do not
have to be supplied by a different V4L2 driver, they can also be
supplied by an independent source. In this case the standart kernel
clock API should be used to handle such clocks. This patch adds support
for such cases.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/v4l2-core/v4l2-clk.c | 76 +++++++++++++++++++++++++++++++++-----
 include/media/v4l2-clk.h           |  2 +
 2 files changed, 68 insertions(+), 10 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
index c210906..693e5a0 100644
--- a/drivers/media/v4l2-core/v4l2-clk.c
+++ b/drivers/media/v4l2-core/v4l2-clk.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/atomic.h>
+#include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/list.h>
@@ -39,9 +40,24 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id)
 	return ERR_PTR(-ENODEV);
 }
 
+static int v4l2_clk_add(struct v4l2_clk *clk, const char *dev_id,
+			const char *id)
+{
+	mutex_lock(&clk_lock);
+	if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
+		mutex_unlock(&clk_lock);
+		return -EEXIST;
+	}
+	list_add_tail(&clk->list, &clk_list);
+	mutex_unlock(&clk_lock);
+
+	return 0;
+}
+
 struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id)
 {
 	struct v4l2_clk *clk;
+	struct clk *ccf_clk = clk_get(dev, id);
 
 	mutex_lock(&clk_lock);
 	clk = v4l2_clk_find(dev_name(dev), id);
@@ -50,6 +66,22 @@ struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id)
 		atomic_inc(&clk->use_count);
 	mutex_unlock(&clk_lock);
 
+	if (!IS_ERR(ccf_clk) && IS_ERR(clk)) {
+		/* not yet on the list */
+		clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
+		if (clk)
+			clk->id = kstrdup(id, GFP_KERNEL);
+		if (!clk || !clk->id) {
+			kfree(clk);
+			clk_put(ccf_clk);
+			return ERR_PTR(-ENOMEM);
+		}
+		clk->clk = ccf_clk;
+		atomic_set(&clk->use_count, 1);
+
+		v4l2_clk_add(clk, dev_name(dev), id);
+	}
+
 	return clk;
 }
 EXPORT_SYMBOL(v4l2_clk_get);
@@ -67,6 +99,15 @@ void v4l2_clk_put(struct v4l2_clk *clk)
 		if (tmp == clk)
 			atomic_dec(&clk->use_count);
 
+	if (clk->clk) {
+		clk_put(clk->clk);
+		if (!atomic_read(&clk->use_count)) {
+			list_del(&clk->list);
+			kfree(clk->id);
+			kfree(clk);
+		}
+	}
+
 	mutex_unlock(&clk_lock);
 }
 EXPORT_SYMBOL(v4l2_clk_put);
@@ -98,8 +139,12 @@ static void v4l2_clk_unlock_driver(struct v4l2_clk *clk)
 
 int v4l2_clk_enable(struct v4l2_clk *clk)
 {
-	int ret = v4l2_clk_lock_driver(clk);
+	int ret;
+
+	if (clk->clk)
+		return clk_enable(clk->clk);
 
+	ret = v4l2_clk_lock_driver(clk);
 	if (ret < 0)
 		return ret;
 
@@ -125,6 +170,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
 {
 	int enable;
 
+	if (clk->clk)
+		return clk_disable(clk->clk);
+
 	mutex_lock(&clk->lock);
 
 	enable = --clk->enable;
@@ -142,8 +190,12 @@ EXPORT_SYMBOL(v4l2_clk_disable);
 
 unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
 {
-	int ret = v4l2_clk_lock_driver(clk);
+	int ret;
 
+	if (clk->clk)
+		return clk_get_rate(clk->clk);
+
+	ret = v4l2_clk_lock_driver(clk);
 	if (ret < 0)
 		return ret;
 
@@ -162,7 +214,16 @@ EXPORT_SYMBOL(v4l2_clk_get_rate);
 
 int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
 {
-	int ret = v4l2_clk_lock_driver(clk);
+	int ret;
+
+	if (clk->clk) {
+		long r = clk_round_rate(clk->clk, rate);
+		if (r < 0)
+			return r;
+		return clk_set_rate(clk->clk, r);
+	}
+
+	ret = v4l2_clk_lock_driver(clk);
 
 	if (ret < 0)
 		return ret;
@@ -204,14 +265,9 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
 	atomic_set(&clk->use_count, 0);
 	mutex_init(&clk->lock);
 
-	mutex_lock(&clk_lock);
-	if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) {
-		mutex_unlock(&clk_lock);
-		ret = -EEXIST;
+	ret = v4l2_clk_add(clk, dev_id, NULL);
+	if (ret < 0)
 		goto eexist;
-	}
-	list_add_tail(&clk->list, &clk_list);
-	mutex_unlock(&clk_lock);
 
 	return clk;
 
diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
index 8f06967..4402b2d 100644
--- a/include/media/v4l2-clk.h
+++ b/include/media/v4l2-clk.h
@@ -22,6 +22,7 @@
 struct module;
 struct device;
 
+struct clk;
 struct v4l2_clk {
 	struct list_head list;
 	const struct v4l2_clk_ops *ops;
@@ -30,6 +31,7 @@ struct v4l2_clk {
 	int enable;
 	struct mutex lock; /* Protect the enable count */
 	atomic_t use_count;
+	struct clk *clk;
 	void *priv;
 };
 
-- 
1.9.3

Re: [PATCH 2/2] V4L2: add CCF support to the v4l2_clk API

[Laurent Pinchart]

Hi Guennadi,

Thank you for the patch. I like the approach.

On Friday 02 January 2015 12:48:43 Guennadi Liakhovetski wrote:
> V4L2 clocks, e.g. used by camera sensors for their master clock, do not
> have to be supplied by a different V4L2 driver, they can also be
> supplied by an independent source. In this case the standart kernel
> clock API should be used to handle such clocks. This patch adds support
> for such cases.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/v4l2-core/v4l2-clk.c | 76 ++++++++++++++++++++++++++++++-----
>  include/media/v4l2-clk.h           |  2 +
>  2 files changed, 68 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> b/drivers/media/v4l2-core/v4l2-clk.c index c210906..693e5a0 100644
> --- a/drivers/media/v4l2-core/v4l2-clk.c
> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> @@ -9,6 +9,7 @@
>   */
> 
>  #include <linux/atomic.h>
> +#include <linux/clk.h>
>  #include <linux/device.h>
>  #include <linux/errno.h>
>  #include <linux/list.h>
> @@ -39,9 +40,24 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id,
> const char *id) return ERR_PTR(-ENODEV);
>  }
> 
> +static int v4l2_clk_add(struct v4l2_clk *clk, const char *dev_id,
> +			const char *id)
> +{
> +	mutex_lock(&clk_lock);
> +	if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
> +		mutex_unlock(&clk_lock);
> +		return -EEXIST;
> +	}
> +	list_add_tail(&clk->list, &clk_list);
> +	mutex_unlock(&clk_lock);
> +
> +	return 0;
> +}
> +
>  struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id)
>  {
>  	struct v4l2_clk *clk;
> +	struct clk *ccf_clk = clk_get(dev, id);
> 
>  	mutex_lock(&clk_lock);
>  	clk = v4l2_clk_find(dev_name(dev), id);
> @@ -50,6 +66,22 @@ struct v4l2_clk *v4l2_clk_get(struct device *dev, const
> char *id) atomic_inc(&clk->use_count);
>  	mutex_unlock(&clk_lock);

Shouldn't we start by CCF and then fall back to V4L2 clocks ?

> +	if (!IS_ERR(ccf_clk) && IS_ERR(clk)) {
> +		/* not yet on the list */
> +		clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
> +		if (clk)
> +			clk->id = kstrdup(id, GFP_KERNEL);
> +		if (!clk || !clk->id) {
> +			kfree(clk);
> +			clk_put(ccf_clk);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +		clk->clk = ccf_clk;
> +		atomic_set(&clk->use_count, 1);
> +
> +		v4l2_clk_add(clk, dev_name(dev), id);

I don't think there's a need to add this clock to the clk_list. As all 
operations are delegated to CCF, and as CCF implements proper refcounting, we 
can just create one v4l2_clk wrapper instance for every call to 
v4l2_clk_get().

> +	}
> +
>  	return clk;
>  }
>  EXPORT_SYMBOL(v4l2_clk_get);
> @@ -67,6 +99,15 @@ void v4l2_clk_put(struct v4l2_clk *clk)
>  		if (tmp == clk)
>  			atomic_dec(&clk->use_count);
> 
> +	if (clk->clk) {
> +		clk_put(clk->clk);
> +		if (!atomic_read(&clk->use_count)) {
> +			list_del(&clk->list);
> +			kfree(clk->id);
> +			kfree(clk);
> +		}
> +	}
> +
>  	mutex_unlock(&clk_lock);
>  }
>  EXPORT_SYMBOL(v4l2_clk_put);
> @@ -98,8 +139,12 @@ static void v4l2_clk_unlock_driver(struct v4l2_clk *clk)
> 
>  int v4l2_clk_enable(struct v4l2_clk *clk)
>  {
> -	int ret = v4l2_clk_lock_driver(clk);
> +	int ret;
> +
> +	if (clk->clk)
> +		return clk_enable(clk->clk);
> 
> +	ret = v4l2_clk_lock_driver(clk);
>  	if (ret < 0)
>  		return ret;
> 
> @@ -125,6 +170,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
>  {
>  	int enable;
> 
> +	if (clk->clk)
> +		return clk_disable(clk->clk);
> +
>  	mutex_lock(&clk->lock);
> 
>  	enable = --clk->enable;
> @@ -142,8 +190,12 @@ EXPORT_SYMBOL(v4l2_clk_disable);
> 
>  unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
>  {
> -	int ret = v4l2_clk_lock_driver(clk);
> +	int ret;
> 
> +	if (clk->clk)
> +		return clk_get_rate(clk->clk);
> +
> +	ret = v4l2_clk_lock_driver(clk);
>  	if (ret < 0)
>  		return ret;
> 
> @@ -162,7 +214,16 @@ EXPORT_SYMBOL(v4l2_clk_get_rate);
> 
>  int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
>  {
> -	int ret = v4l2_clk_lock_driver(clk);
> +	int ret;
> +
> +	if (clk->clk) {
> +		long r = clk_round_rate(clk->clk, rate);
> +		if (r < 0)
> +			return r;
> +		return clk_set_rate(clk->clk, r);
> +	}
> +
> +	ret = v4l2_clk_lock_driver(clk);
> 
>  	if (ret < 0)
>  		return ret;
> @@ -204,14 +265,9 @@ struct v4l2_clk *v4l2_clk_register(const struct
> v4l2_clk_ops *ops, atomic_set(&clk->use_count, 0);
>  	mutex_init(&clk->lock);
> 
> -	mutex_lock(&clk_lock);
> -	if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) {
> -		mutex_unlock(&clk_lock);
> -		ret = -EEXIST;
> +	ret = v4l2_clk_add(clk, dev_id, NULL);
> +	if (ret < 0)
>  		goto eexist;
> -	}
> -	list_add_tail(&clk->list, &clk_list);
> -	mutex_unlock(&clk_lock);
> 
>  	return clk;
> 
> diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> index 8f06967..4402b2d 100644
> --- a/include/media/v4l2-clk.h
> +++ b/include/media/v4l2-clk.h
> @@ -22,6 +22,7 @@
>  struct module;
>  struct device;
> 
> +struct clk;
>  struct v4l2_clk {
>  	struct list_head list;
>  	const struct v4l2_clk_ops *ops;
> @@ -30,6 +31,7 @@ struct v4l2_clk {
>  	int enable;
>  	struct mutex lock; /* Protect the enable count */
>  	atomic_t use_count;
> +	struct clk *clk;
>  	void *priv;
>  };

-- 
Regards,

Laurent Pinchart

[PATCH v2 2/2] V4L2: add CCF support to the v4l2_clk API

[Guennadi Liakhovetski]

>From aeaee56e04d023f3a019d2595ef5128015acdb06 Mon Sep 17 00:00:00 2001
From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Date: Fri, 2 Jan 2015 12:26:41 +0100
Subject: [PATCH 2/2] V4L2: add CCF support to the v4l2_clk API

V4L2 clocks, e.g. used by camera sensors for their master clock, do not
have to be supplied by a different V4L2 driver, they can also be
supplied by an independent source. In this case the standart kernel
clock API should be used to handle such clocks. This patch adds support
for such cases.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

Hi Laurent,
Thanks for the comment. The idea of allocating a new object for each "get" 
operation seems a bit weird to me, and completely trusting the user is a 
bit scary... :) But yes, it can work this way too, I think, and the user 
can screw either way too, anyway. So, here comes a v2. Something like 
this?

v2: don't add CCF-related clocks on the global list, just allocate a new 
instance on each v4l2_clk_get()

 drivers/media/v4l2-core/v4l2-clk.c | 45 +++++++++++++++++++++++++++++++++++---
 include/media/v4l2-clk.h           |  2 ++
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
index c210906..f5d1688 100644
--- a/drivers/media/v4l2-core/v4l2-clk.c
+++ b/drivers/media/v4l2-core/v4l2-clk.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/atomic.h>
+#include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/list.h>
@@ -42,6 +43,18 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id)
 struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id)
 {
 	struct v4l2_clk *clk;
+	struct clk *ccf_clk = clk_get(dev, id);
+
+	if (!IS_ERR(ccf_clk)) {
+		clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
+		if (!clk) {
+			clk_put(ccf_clk);
+			return ERR_PTR(-ENOMEM);
+		}
+		clk->clk = ccf_clk;
+
+		return clk;
+	}
 
 	mutex_lock(&clk_lock);
 	clk = v4l2_clk_find(dev_name(dev), id);
@@ -61,6 +74,12 @@ void v4l2_clk_put(struct v4l2_clk *clk)
 	if (IS_ERR(clk))
 		return;
 
+	if (clk->clk) {
+		clk_put(clk->clk);
+		kfree(clk);
+		return;
+	}
+
 	mutex_lock(&clk_lock);
 
 	list_for_each_entry(tmp, &clk_list, list)
@@ -98,8 +117,12 @@ static void v4l2_clk_unlock_driver(struct v4l2_clk *clk)
 
 int v4l2_clk_enable(struct v4l2_clk *clk)
 {
-	int ret = v4l2_clk_lock_driver(clk);
+	int ret;
 
+	if (clk->clk)
+		return clk_enable(clk->clk);
+
+	ret = v4l2_clk_lock_driver(clk);
 	if (ret < 0)
 		return ret;
 
@@ -125,6 +148,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
 {
 	int enable;
 
+	if (clk->clk)
+		return clk_disable(clk->clk);
+
 	mutex_lock(&clk->lock);
 
 	enable = --clk->enable;
@@ -142,8 +168,12 @@ EXPORT_SYMBOL(v4l2_clk_disable);
 
 unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
 {
-	int ret = v4l2_clk_lock_driver(clk);
+	int ret;
+
+	if (clk->clk)
+		return clk_get_rate(clk->clk);
 
+	ret = v4l2_clk_lock_driver(clk);
 	if (ret < 0)
 		return ret;
 
@@ -162,7 +192,16 @@ EXPORT_SYMBOL(v4l2_clk_get_rate);
 
 int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
 {
-	int ret = v4l2_clk_lock_driver(clk);
+	int ret;
+
+	if (clk->clk) {
+		long r = clk_round_rate(clk->clk, rate);
+		if (r < 0)
+			return r;
+		return clk_set_rate(clk->clk, r);
+	}
+
+	ret = v4l2_clk_lock_driver(clk);
 
 	if (ret < 0)
 		return ret;
diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
index 8f06967..4402b2d 100644
--- a/include/media/v4l2-clk.h
+++ b/include/media/v4l2-clk.h
@@ -22,6 +22,7 @@
 struct module;
 struct device;
 
+struct clk;
 struct v4l2_clk {
 	struct list_head list;
 	const struct v4l2_clk_ops *ops;
@@ -30,6 +31,7 @@ struct v4l2_clk {
 	int enable;
 	struct mutex lock; /* Protect the enable count */
 	atomic_t use_count;
+	struct clk *clk;
 	void *priv;
 };
 
-- 
1.9.3

Re: [PATCH 0/2] V4L2: add CCF support to v4l2_clk

[Josh Wu]

Hi, Guennadi

On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote:
> Hi,
>
> This is an attempt to implement CCF support for v4l2_clk to be able to use
> e.g. DT-based clocks. Beware - completely untested! Josh, could you please
> see, whether you can use this as a starting point?

Thanks for the patches. I will test these patches tomorrow.

Best Regards,
Josh Wu

>
> Thanks
> Guennadi

Re: [PATCH v2 2/2] V4L2: add CCF support to the v4l2_clk API

[Laurent Pinchart]

Hi Guennadi,

Thank you for the patch.

On Friday 02 January 2015 21:18:41 Guennadi Liakhovetski wrote:
> From aeaee56e04d023f3a019d2595ef5128015acdb06 Mon Sep 17 00:00:00 2001
> From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Date: Fri, 2 Jan 2015 12:26:41 +0100
> Subject: [PATCH 2/2] V4L2: add CCF support to the v4l2_clk API
> 
> V4L2 clocks, e.g. used by camera sensors for their master clock, do not
> have to be supplied by a different V4L2 driver, they can also be
> supplied by an independent source. In this case the standart kernel
> clock API should be used to handle such clocks. This patch adds support
> for such cases.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> Hi Laurent,
> Thanks for the comment. The idea of allocating a new object for each "get"
> operation seems a bit weird to me, and completely trusting the user is a
> bit scary... :) But yes, it can work this way too, I think, and the user
> can screw either way too, anyway.

The user needs to get it right for when we'll replace v4l2_clk_get() with 
clk_get(), so I'd rather force it to get it right now.

> So, here comes a v2. Something like this?
> 
> v2: don't add CCF-related clocks on the global list, just allocate a new
> instance on each v4l2_clk_get()
> 
>  drivers/media/v4l2-core/v4l2-clk.c | 45 +++++++++++++++++++++++++++++++---
>  include/media/v4l2-clk.h           |  2 ++
>  2 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> b/drivers/media/v4l2-core/v4l2-clk.c index c210906..f5d1688 100644
> --- a/drivers/media/v4l2-core/v4l2-clk.c
> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> @@ -9,6 +9,7 @@
>   */
> 
>  #include <linux/atomic.h>
> +#include <linux/clk.h>
>  #include <linux/device.h>
>  #include <linux/errno.h>
>  #include <linux/list.h>
> @@ -42,6 +43,18 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id,
> const char *id) struct v4l2_clk *v4l2_clk_get(struct device *dev, const
> char *id) {
>  	struct v4l2_clk *clk;
> +	struct clk *ccf_clk = clk_get(dev, id);
> +
> +	if (!IS_ERR(ccf_clk)) {
> +		clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
> +		if (!clk) {
> +			clk_put(ccf_clk);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +		clk->clk = ccf_clk;
> +
> +		return clk;
> +	}

If clk_get() returns -EPROBE_DEFER I think you should return the error code to 
the user instead of falling back to the v4l2 clocks.

>  	mutex_lock(&clk_lock);
>  	clk = v4l2_clk_find(dev_name(dev), id);
> @@ -61,6 +74,12 @@ void v4l2_clk_put(struct v4l2_clk *clk)
>  	if (IS_ERR(clk))
>  		return;
> 
> +	if (clk->clk) {
> +		clk_put(clk->clk);
> +		kfree(clk);
> +		return;
> +	}
> +
>  	mutex_lock(&clk_lock);
> 
>  	list_for_each_entry(tmp, &clk_list, list)
> @@ -98,8 +117,12 @@ static void v4l2_clk_unlock_driver(struct v4l2_clk *clk)
> 
>  int v4l2_clk_enable(struct v4l2_clk *clk)
>  {
> -	int ret = v4l2_clk_lock_driver(clk);
> +	int ret;
> 
> +	if (clk->clk)
> +		return clk_enable(clk->clk);

Shouldn't you use clk_prepare_enable() ? CCF requires a prepare call before 
enabling the clock.

> +
> +	ret = v4l2_clk_lock_driver(clk);
>  	if (ret < 0)
>  		return ret;
> 
> @@ -125,6 +148,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
>  {
>  	int enable;
> 
> +	if (clk->clk)
> +		return clk_disable(clk->clk);

Likewise, clk_disable_unprepare() ?

> +
>  	mutex_lock(&clk->lock);
> 
>  	enable = --clk->enable;
> @@ -142,8 +168,12 @@ EXPORT_SYMBOL(v4l2_clk_disable);
> 
>  unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
>  {
> -	int ret = v4l2_clk_lock_driver(clk);
> +	int ret;
> +
> +	if (clk->clk)
> +		return clk_get_rate(clk->clk);
> 
> +	ret = v4l2_clk_lock_driver(clk);
>  	if (ret < 0)
>  		return ret;
> 
> @@ -162,7 +192,16 @@ EXPORT_SYMBOL(v4l2_clk_get_rate);
> 
>  int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
>  {
> -	int ret = v4l2_clk_lock_driver(clk);
> +	int ret;
> +
> +	if (clk->clk) {
> +		long r = clk_round_rate(clk->clk, rate);
> +		if (r < 0)
> +			return r;
> +		return clk_set_rate(clk->clk, r);
> +	}
> +
> +	ret = v4l2_clk_lock_driver(clk);
> 
>  	if (ret < 0)
>  		return ret;
> diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> index 8f06967..4402b2d 100644
> --- a/include/media/v4l2-clk.h
> +++ b/include/media/v4l2-clk.h
> @@ -22,6 +22,7 @@
>  struct module;
>  struct device;
> 
> +struct clk;
>  struct v4l2_clk {
>  	struct list_head list;
>  	const struct v4l2_clk_ops *ops;
> @@ -30,6 +31,7 @@ struct v4l2_clk {
>  	int enable;
>  	struct mutex lock; /* Protect the enable count */
>  	atomic_t use_count;
> +	struct clk *clk;
>  	void *priv;
>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/2] V4L2: add CCF support to the v4l2_clk API

[Guennadi Liakhovetski]

On Sun, 4 Jan 2015, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thank you for the patch.
> 
> On Friday 02 January 2015 21:18:41 Guennadi Liakhovetski wrote:
> > From aeaee56e04d023f3a019d2595ef5128015acdb06 Mon Sep 17 00:00:00 2001
> > From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Date: Fri, 2 Jan 2015 12:26:41 +0100
> > Subject: [PATCH 2/2] V4L2: add CCF support to the v4l2_clk API
> > 
> > V4L2 clocks, e.g. used by camera sensors for their master clock, do not
> > have to be supplied by a different V4L2 driver, they can also be
> > supplied by an independent source. In this case the standart kernel
> > clock API should be used to handle such clocks. This patch adds support
> > for such cases.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > 
> > Hi Laurent,
> > Thanks for the comment. The idea of allocating a new object for each "get"
> > operation seems a bit weird to me, and completely trusting the user is a
> > bit scary... :) But yes, it can work this way too, I think, and the user
> > can screw either way too, anyway.
> 
> The user needs to get it right for when we'll replace v4l2_clk_get() with 
> clk_get(), so I'd rather force it to get it right now.
> 
> > So, here comes a v2. Something like this?
> > 
> > v2: don't add CCF-related clocks on the global list, just allocate a new
> > instance on each v4l2_clk_get()
> > 
> >  drivers/media/v4l2-core/v4l2-clk.c | 45 +++++++++++++++++++++++++++++++---
> >  include/media/v4l2-clk.h           |  2 ++
> >  2 files changed, 44 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> > b/drivers/media/v4l2-core/v4l2-clk.c index c210906..f5d1688 100644
> > --- a/drivers/media/v4l2-core/v4l2-clk.c
> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> > @@ -9,6 +9,7 @@
> >   */
> > 
> >  #include <linux/atomic.h>
> > +#include <linux/clk.h>
> >  #include <linux/device.h>
> >  #include <linux/errno.h>
> >  #include <linux/list.h>
> > @@ -42,6 +43,18 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id,
> > const char *id) struct v4l2_clk *v4l2_clk_get(struct device *dev, const
> > char *id) {
> >  	struct v4l2_clk *clk;
> > +	struct clk *ccf_clk = clk_get(dev, id);
> > +
> > +	if (!IS_ERR(ccf_clk)) {
> > +		clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
> > +		if (!clk) {
> > +			clk_put(ccf_clk);
> > +			return ERR_PTR(-ENOMEM);
> > +		}
> > +		clk->clk = ccf_clk;
> > +
> > +		return clk;
> > +	}
> 
> If clk_get() returns -EPROBE_DEFER I think you should return the error code to 
> the user instead of falling back to the v4l2 clocks.

This is the case, when a clock is correctly set up in DT, but no suitable 
provider is registered yet, right? I think such a case isn't very likely, 
but yes, can do, makes more sense, perhaps.

> >  	mutex_lock(&clk_lock);
> >  	clk = v4l2_clk_find(dev_name(dev), id);
> > @@ -61,6 +74,12 @@ void v4l2_clk_put(struct v4l2_clk *clk)
> >  	if (IS_ERR(clk))
> >  		return;
> > 
> > +	if (clk->clk) {
> > +		clk_put(clk->clk);
> > +		kfree(clk);
> > +		return;
> > +	}
> > +
> >  	mutex_lock(&clk_lock);
> > 
> >  	list_for_each_entry(tmp, &clk_list, list)
> > @@ -98,8 +117,12 @@ static void v4l2_clk_unlock_driver(struct v4l2_clk *clk)
> > 
> >  int v4l2_clk_enable(struct v4l2_clk *clk)
> >  {
> > -	int ret = v4l2_clk_lock_driver(clk);
> > +	int ret;
> > 
> > +	if (clk->clk)
> > +		return clk_enable(clk->clk);
> 
> Shouldn't you use clk_prepare_enable() ? CCF requires a prepare call before 
> enabling the clock.

Right, my fault, thanks for pointing out.

> > +
> > +	ret = v4l2_clk_lock_driver(clk);
> >  	if (ret < 0)
> >  		return ret;
> > 
> > @@ -125,6 +148,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
> >  {
> >  	int enable;
> > 
> > +	if (clk->clk)
> > +		return clk_disable(clk->clk);
> 
> Likewise, clk_disable_unprepare() ?

Sure. Ok, these are trivial changes, perhaps we can wait for Josh's test 
results, then I can submit a v3 with (hopefully) all required changes.

Thanks
Guennadi

> > +
> >  	mutex_lock(&clk->lock);
> > 
> >  	enable = --clk->enable;
> > @@ -142,8 +168,12 @@ EXPORT_SYMBOL(v4l2_clk_disable);
> > 
> >  unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
> >  {
> > -	int ret = v4l2_clk_lock_driver(clk);
> > +	int ret;
> > +
> > +	if (clk->clk)
> > +		return clk_get_rate(clk->clk);
> > 
> > +	ret = v4l2_clk_lock_driver(clk);
> >  	if (ret < 0)
> >  		return ret;
> > 
> > @@ -162,7 +192,16 @@ EXPORT_SYMBOL(v4l2_clk_get_rate);
> > 
> >  int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
> >  {
> > -	int ret = v4l2_clk_lock_driver(clk);
> > +	int ret;
> > +
> > +	if (clk->clk) {
> > +		long r = clk_round_rate(clk->clk, rate);
> > +		if (r < 0)
> > +			return r;
> > +		return clk_set_rate(clk->clk, r);
> > +	}
> > +
> > +	ret = v4l2_clk_lock_driver(clk);
> > 
> >  	if (ret < 0)
> >  		return ret;
> > diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> > index 8f06967..4402b2d 100644
> > --- a/include/media/v4l2-clk.h
> > +++ b/include/media/v4l2-clk.h
> > @@ -22,6 +22,7 @@
> >  struct module;
> >  struct device;
> > 
> > +struct clk;
> >  struct v4l2_clk {
> >  	struct list_head list;
> >  	const struct v4l2_clk_ops *ops;
> > @@ -30,6 +31,7 @@ struct v4l2_clk {
> >  	int enable;
> >  	struct mutex lock; /* Protect the enable count */
> >  	atomic_t use_count;
> > +	struct clk *clk;
> >  	void *priv;
> >  };
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH v2 2/2] V4L2: add CCF support to the v4l2_clk API

[Laurent Pinchart]

Hi Guennadi,

On Sunday 04 January 2015 17:45:04 Guennadi Liakhovetski wrote:
> On Sun, 4 Jan 2015, Laurent Pinchart wrote:
> > On Friday 02 January 2015 21:18:41 Guennadi Liakhovetski wrote:
> > > From aeaee56e04d023f3a019d2595ef5128015acdb06 Mon Sep 17 00:00:00 2001
> > > From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > Date: Fri, 2 Jan 2015 12:26:41 +0100
> > > Subject: [PATCH 2/2] V4L2: add CCF support to the v4l2_clk API
> > > 
> > > V4L2 clocks, e.g. used by camera sensors for their master clock, do not
> > > have to be supplied by a different V4L2 driver, they can also be
> > > supplied by an independent source. In this case the standart kernel
> > > clock API should be used to handle such clocks. This patch adds support
> > > for such cases.
> > > 
> > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > ---
> > > 
> > > Hi Laurent,
> > > Thanks for the comment. The idea of allocating a new object for each
> > > "get" operation seems a bit weird to me, and completely trusting the
> > > user is a bit scary... :) But yes, it can work this way too, I think,
> > > and the user can screw either way too, anyway.
> > 
> > The user needs to get it right for when we'll replace v4l2_clk_get() with
> > clk_get(), so I'd rather force it to get it right now.
> > 
> > > So, here comes a v2. Something like this?
> > > 
> > > v2: don't add CCF-related clocks on the global list, just allocate a new
> > > instance on each v4l2_clk_get()
> > > 
> > >  drivers/media/v4l2-core/v4l2-clk.c | 45 ++++++++++++++++++++++++++++---
> > >  include/media/v4l2-clk.h           |  2 ++
> > >  2 files changed, 44 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> > > b/drivers/media/v4l2-core/v4l2-clk.c index c210906..f5d1688 100644
> > > --- a/drivers/media/v4l2-core/v4l2-clk.c
> > > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> > > @@ -9,6 +9,7 @@
> > >   */
> > >  
> > >  #include <linux/atomic.h>
> > > +#include <linux/clk.h>
> > >  #include <linux/device.h>
> > >  #include <linux/errno.h>
> > >  #include <linux/list.h>
> > > @@ -42,6 +43,18 @@ static struct v4l2_clk *v4l2_clk_find(const char
> > > *dev_id, const char *id) struct v4l2_clk *v4l2_clk_get(struct device
> > > *dev, const char *id) {
> > >  	struct v4l2_clk *clk;
> > > +	struct clk *ccf_clk = clk_get(dev, id);
> > > +
> > > +	if (!IS_ERR(ccf_clk)) {
> > > +		clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
> > > +		if (!clk) {
> > > +			clk_put(ccf_clk);
> > > +			return ERR_PTR(-ENOMEM);
> > > +		}
> > > +		clk->clk = ccf_clk;
> > > +
> > > +		return clk;
> > > +	}
> > 
> > If clk_get() returns -EPROBE_DEFER I think you should return the error
> > code to the user instead of falling back to the v4l2 clocks.
> 
> This is the case, when a clock is correctly set up in DT, but no suitable
> provider is registered yet, right? I think such a case isn't very likely,
> but yes, can do, makes more sense, perhaps.

Yes, that's the use case. Even if it's not that likely I think it should still 
be implemented, if only to mimic the CCF behaviour and prepare drivers for the 
switch away from v4l2_clk_get().

> > >  	mutex_lock(&clk_lock);
> > >  	clk = v4l2_clk_find(dev_name(dev), id);
> > > 
> > > @@ -61,6 +74,12 @@ void v4l2_clk_put(struct v4l2_clk *clk)
> > > 
> > >  	if (IS_ERR(clk))
> > >  	
> > >  		return;
> > > 
> > > +	if (clk->clk) {
> > > +		clk_put(clk->clk);
> > > +		kfree(clk);
> > > +		return;
> > > +	}
> > > +
> > > 
> > >  	mutex_lock(&clk_lock);
> > >  	
> > >  	list_for_each_entry(tmp, &clk_list, list)
> > > 
> > > @@ -98,8 +117,12 @@ static void v4l2_clk_unlock_driver(struct v4l2_clk
> > > *clk)> > 
> > >  int v4l2_clk_enable(struct v4l2_clk *clk)
> > >  {
> > > 
> > > -	int ret = v4l2_clk_lock_driver(clk);
> > > +	int ret;
> > > 
> > > +	if (clk->clk)
> > > +		return clk_enable(clk->clk);
> > 
> > Shouldn't you use clk_prepare_enable() ? CCF requires a prepare call
> > before enabling the clock.
> 
> Right, my fault, thanks for pointing out.
> 
> > > +
> > > +	ret = v4l2_clk_lock_driver(clk);
> > > 
> > >  	if (ret < 0)
> > >  	
> > >  		return ret;
> > > 
> > > @@ -125,6 +148,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
> > > 
> > >  {
> > >  
> > >  	int enable;
> > > 
> > > +	if (clk->clk)
> > > +		return clk_disable(clk->clk);
> > 
> > Likewise, clk_disable_unprepare() ?
> 
> Sure. Ok, these are trivial changes, perhaps we can wait for Josh's test
> results, then I can submit a v3 with (hopefully) all required changes.

Sure. CCF will trigger warnings due to clk_prepare() not being called though.

> > > +
> > > 
> > >  	mutex_lock(&clk->lock);
> > >  	
> > >  	enable = --clk->enable;
> > > 
> > > @@ -142,8 +168,12 @@ EXPORT_SYMBOL(v4l2_clk_disable);
> > > 
> > >  unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
> > >  {
> > > 
> > > -	int ret = v4l2_clk_lock_driver(clk);
> > > +	int ret;
> > > +
> > > +	if (clk->clk)
> > > +		return clk_get_rate(clk->clk);
> > > 
> > > +	ret = v4l2_clk_lock_driver(clk);
> > > 
> > >  	if (ret < 0)
> > >  	
> > >  		return ret;
> > > 
> > > @@ -162,7 +192,16 @@ EXPORT_SYMBOL(v4l2_clk_get_rate);
> > > 
> > >  int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
> > >  {
> > > 
> > > -	int ret = v4l2_clk_lock_driver(clk);
> > > +	int ret;
> > > +
> > > +	if (clk->clk) {
> > > +		long r = clk_round_rate(clk->clk, rate);
> > > +		if (r < 0)
> > > +			return r;
> > > +		return clk_set_rate(clk->clk, r);
> > > +	}
> > > +
> > > +	ret = v4l2_clk_lock_driver(clk);
> > > 
> > >  	if (ret < 0)
> > >  	
> > >  		return ret;
> > > 
> > > diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> > > index 8f06967..4402b2d 100644
> > > --- a/include/media/v4l2-clk.h
> > > +++ b/include/media/v4l2-clk.h
> > > @@ -22,6 +22,7 @@
> > > 
> > >  struct module;
> > >  struct device;
> > > 
> > > +struct clk;
> > > 
> > >  struct v4l2_clk {
> > >  
> > >  	struct list_head list;
> > >  	const struct v4l2_clk_ops *ops;
> > > 
> > > @@ -30,6 +31,7 @@ struct v4l2_clk {
> > > 
> > >  	int enable;
> > >  	struct mutex lock; /* Protect the enable count */
> > >  	atomic_t use_count;
> > > 
> > > +	struct clk *clk;
> > > 
> > >  	void *priv;
> > >  
> > >  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] V4L: remove clock name from v4l2_clk API

[Josh Wu]

Hi, Guennadi

On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote:
> All uses of the v4l2_clk API so far only register one clock with a fixed
> name. This allows us to get rid of it, which also will make CCF and DT
> integration easier.
This patch not register a v4l2_clk with name: "mclk". So this will break 
all the i2c sensors that used the v4l2 clock "mclk".
To make those drivers work, we need change all lines from:
    v4l2_clk_get(&client->dev, "mclk");
to:
    v4l2_clk_get(&client->dev, NULL);

Am I understanding correctly?

if yes, I'd like to define a SOC_CAMERA_HOST_CLK_ID in soc_camera.h, and 
all code will use:
     v4l2_clk_get(&client->dev, SOC_CAMERA_HOST_CLK_ID);

>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>   drivers/media/platform/soc_camera/soc_camera.c |  6 +++---
>   drivers/media/usb/em28xx/em28xx-camera.c       |  2 +-
>   drivers/media/v4l2-core/v4l2-clk.c             | 24 +++++++++++-------------
>   include/media/v4l2-clk.h                       |  7 +++----
>   4 files changed, 18 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index f4be2a1..ce192b6 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct soc_camera_device *icd,
>   	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
>   		 shd->i2c_adapter_id, shd->board_info->addr);
>   
> -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd);
> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>   	if (IS_ERR(icd->clk)) {
>   		ret = PTR_ERR(icd->clk);
>   		goto eclkreg;
> @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host *ici,
>   	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
>   		 sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address);
>   
> -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd);
> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>   	if (IS_ERR(icd->clk)) {
>   		ret = PTR_ERR(icd->clk);
>   		goto eclkreg;
> @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
>   		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);
> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>   	if (IS_ERR(icd->clk)) {
>   		ret = PTR_ERR(icd->clk);
>   		goto eclkreg;
> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
> index 7be661f..a4b22c2 100644
> --- a/drivers/media/usb/em28xx/em28xx-camera.c
> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev)
>   
>   	v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
>   			  i2c_adapter_id(adap), client->addr);
> -	v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL);
> +	v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL);
>   	if (IS_ERR(v4l2->clk))
>   		return PTR_ERR(v4l2->clk);
>   
> diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> index e18cc04..c210906 100644
> --- a/drivers/media/v4l2-core/v4l2-clk.c
> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id)
>   		if (strcmp(dev_id, clk->dev_id))
>   			continue;
>   
> -		if (!id || !clk->id || !strcmp(clk->id, id))
> +		if ((!id && !clk->id) ||
> +		    (id && clk->id && !strcmp(clk->id, id)))
>   			return clk;
>   	}
>   
> @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
>   	mutex_lock(&clk->lock);
>   
>   	enable = --clk->enable;
> -	if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__,
> -		 clk->dev_id, clk->id))
> +	if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__,
> +		 clk->dev_id))
>   		clk->enable++;
>   	else if (!enable && clk->ops->disable)
>   		clk->ops->disable(clk);
> @@ -181,7 +182,7 @@ EXPORT_SYMBOL(v4l2_clk_set_rate);
>   
>   struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>   				   const char *dev_id,
> -				   const char *id, void *priv)
> +				   void *priv)
>   {
>   	struct v4l2_clk *clk;
>   	int ret;
> @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>   	if (!clk)
>   		return ERR_PTR(-ENOMEM);
>   
> -	clk->id = kstrdup(id, GFP_KERNEL);
>   	clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
> -	if ((id && !clk->id) || !clk->dev_id) {
> +	if (!clk->dev_id) {
>   		ret = -ENOMEM;
>   		goto ealloc;
>   	}
> @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>   	mutex_init(&clk->lock);
>   
>   	mutex_lock(&clk_lock);
> -	if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
> +	if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) {
So the camera sensor driver should request the v4l2 clock with id is NULL?

How about define a SOC_CAMERA_HOST_CLK_ID in soc_camera.h, like:
     #define SOC_CAMERA_HOST_CLK_ID    NULL

then we can rewrite this block code to:

         clk->ops = ops;
         clk->priv = priv;
+      clk->id = SOC_CAMERA_HOST_CLK_ID;
         atomic_set(&clk->use_count, 0);
         mutex_init(&clk->lock);

         mutex_lock(&clk_lock);
         if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
         ... ...

Best Regards,
Josh Wu

>   		mutex_unlock(&clk_lock);
>   		ret = -EEXIST;
>   		goto eexist;
> @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>   
>   eexist:
>   ealloc:
> -	kfree(clk->id);
>   	kfree(clk->dev_id);
>   	kfree(clk);
>   	return ERR_PTR(ret);
> @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register);
>   void v4l2_clk_unregister(struct v4l2_clk *clk)
>   {
>   	if (WARN(atomic_read(&clk->use_count),
> -		 "%s(): Refusing to unregister ref-counted %s:%s clock!\n",
> -		 __func__, clk->dev_id, clk->id))
> +		 "%s(): Refusing to unregister ref-counted %s clock!\n",
> +		 __func__, clk->dev_id))
>   		return;
>   
>   	mutex_lock(&clk_lock);
>   	list_del(&clk->list);
>   	mutex_unlock(&clk_lock);
>   
> -	kfree(clk->id);
>   	kfree(clk->dev_id);
>   	kfree(clk);
>   }
> @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk *clk)
>   }
>   
>   struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
> -		const char *id, unsigned long rate, struct module *owner)
> +				unsigned long rate, struct module *owner)
>   {
>   	struct v4l2_clk *clk;
>   	struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
>   	priv->ops.get_rate = fixed_get_rate;
>   	priv->ops.owner = owner;
>   
> -	clk = v4l2_clk_register(&priv->ops, dev_id, id, priv);
> +	clk = v4l2_clk_register(&priv->ops, dev_id, priv);
>   	if (IS_ERR(clk))
>   		kfree(priv);
>   
> diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> index 0b36cc1..8f06967 100644
> --- a/include/media/v4l2-clk.h
> +++ b/include/media/v4l2-clk.h
> @@ -43,7 +43,7 @@ struct v4l2_clk_ops {
>   
>   struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>   				   const char *dev_name,
> -				   const char *name, void *priv);
> +				   void *priv);
>   void v4l2_clk_unregister(struct v4l2_clk *clk);
>   struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id);
>   void v4l2_clk_put(struct v4l2_clk *clk);
> @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate);
>   struct module;
>   
>   struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
> -		const char *id, unsigned long rate, struct module *owner);
> +			unsigned long rate, struct module *owner);
>   void v4l2_clk_unregister_fixed(struct v4l2_clk *clk);
>   
>   static inline struct v4l2_clk *v4l2_clk_register_fixed(const char *dev_id,
> -							const char *id,
>   							unsigned long rate)
>   {
> -	return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE);
> +	return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE);
>   }
>   
>   #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, \

Re: [PATCH v2 2/2] V4L2: add CCF support to the v4l2_clk API

[Josh Wu]

Hi, Guennadi

On 1/3/2015 4:18 AM, Guennadi Liakhovetski wrote:
>  From aeaee56e04d023f3a019d2595ef5128015acdb06 Mon Sep 17 00:00:00 2001
> From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Date: Fri, 2 Jan 2015 12:26:41 +0100
> Subject: [PATCH 2/2] V4L2: add CCF support to the v4l2_clk API
>
> V4L2 clocks, e.g. used by camera sensors for their master clock, do not
> have to be supplied by a different V4L2 driver, they can also be
> supplied by an independent source. In this case the standart kernel
> clock API should be used to handle such clocks. This patch adds support
> for such cases.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> Hi Laurent,
> Thanks for the comment. The idea of allocating a new object for each "get"
> operation seems a bit weird to me, and completely trusting the user is a
> bit scary... :) But yes, it can work this way too, I think, and the user
> can screw either way too, anyway. So, here comes a v2. Something like
> this?
>
> v2: don't add CCF-related clocks on the global list, just allocate a new
> instance on each v4l2_clk_get()
>
>   drivers/media/v4l2-core/v4l2-clk.c | 45 +++++++++++++++++++++++++++++++++++---
>   include/media/v4l2-clk.h           |  2 ++
>   2 files changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> index c210906..f5d1688 100644
> --- a/drivers/media/v4l2-core/v4l2-clk.c
> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> @@ -9,6 +9,7 @@
>    */
>   
>   #include <linux/atomic.h>
> +#include <linux/clk.h>
>   #include <linux/device.h>
>   #include <linux/errno.h>
>   #include <linux/list.h>
> @@ -42,6 +43,18 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id)
>   struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id)
>   {
>   	struct v4l2_clk *clk;
> +	struct clk *ccf_clk = clk_get(dev, id);
We need to check whether the 'id' is NULL, otherwise there is no error 
reported if we use IS_ERR(ccf_clk).

Best Regards,
Josh Wu

> +
> +	if (!IS_ERR(ccf_clk)) {
> +		clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
> +		if (!clk) {
> +			clk_put(ccf_clk);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +		clk->clk = ccf_clk;
> +
> +		return clk;
> +	}
>   
>   	mutex_lock(&clk_lock);
>   	clk = v4l2_clk_find(dev_name(dev), id);
> @@ -61,6 +74,12 @@ void v4l2_clk_put(struct v4l2_clk *clk)
>   	if (IS_ERR(clk))
>   		return;
>   
> +	if (clk->clk) {
> +		clk_put(clk->clk);
> +		kfree(clk);
> +		return;
> +	}
> +
>   	mutex_lock(&clk_lock);
>   
>   	list_for_each_entry(tmp, &clk_list, list)
> @@ -98,8 +117,12 @@ static void v4l2_clk_unlock_driver(struct v4l2_clk *clk)
>   
>   int v4l2_clk_enable(struct v4l2_clk *clk)
>   {
> -	int ret = v4l2_clk_lock_driver(clk);
> +	int ret;
>   
> +	if (clk->clk)
> +		return clk_enable(clk->clk);
> +
> +	ret = v4l2_clk_lock_driver(clk);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -125,6 +148,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
>   {
>   	int enable;
>   
> +	if (clk->clk)
> +		return clk_disable(clk->clk);
> +
>   	mutex_lock(&clk->lock);
>   
>   	enable = --clk->enable;
> @@ -142,8 +168,12 @@ EXPORT_SYMBOL(v4l2_clk_disable);
>   
>   unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
>   {
> -	int ret = v4l2_clk_lock_driver(clk);
> +	int ret;
> +
> +	if (clk->clk)
> +		return clk_get_rate(clk->clk);
>   
> +	ret = v4l2_clk_lock_driver(clk);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -162,7 +192,16 @@ EXPORT_SYMBOL(v4l2_clk_get_rate);
>   
>   int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
>   {
> -	int ret = v4l2_clk_lock_driver(clk);
> +	int ret;
> +
> +	if (clk->clk) {
> +		long r = clk_round_rate(clk->clk, rate);
> +		if (r < 0)
> +			return r;
> +		return clk_set_rate(clk->clk, r);
> +	}
> +
> +	ret = v4l2_clk_lock_driver(clk);
>   
>   	if (ret < 0)
>   		return ret;
> diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> index 8f06967..4402b2d 100644
> --- a/include/media/v4l2-clk.h
> +++ b/include/media/v4l2-clk.h
> @@ -22,6 +22,7 @@
>   struct module;
>   struct device;
>   
> +struct clk;
>   struct v4l2_clk {
>   	struct list_head list;
>   	const struct v4l2_clk_ops *ops;
> @@ -30,6 +31,7 @@ struct v4l2_clk {
>   	int enable;
>   	struct mutex lock; /* Protect the enable count */
>   	atomic_t use_count;
> +	struct clk *clk;
>   	void *priv;
>   };
>   

Re: [PATCH 1/2] V4L: remove clock name from v4l2_clk API

[Guennadi Liakhovetski]

Hi Josh,

On Mon, 5 Jan 2015, Josh Wu wrote:

> Hi, Guennadi
> 
> On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote:
> > All uses of the v4l2_clk API so far only register one clock with a fixed
> > name. This allows us to get rid of it, which also will make CCF and DT
> > integration easier.
> This patch not register a v4l2_clk with name: "mclk". So this will break all
> the i2c sensors that used the v4l2 clock "mclk".
> To make those drivers work, we need change all lines from:
>    v4l2_clk_get(&client->dev, "mclk");
> to:
>    v4l2_clk_get(&client->dev, NULL);
> 
> Am I understanding correctly?

No, it's the opposite. Clock consumers should request clock names, 
according to their datasheets. In ov2640 case it's xvclk. For v4l2_clk the 
name will simply be ignored. But it will be used to try to get a CCF clock 
before falling back to V4L2 clocks.

Thanks
Guennadi

> if yes, I'd like to define a SOC_CAMERA_HOST_CLK_ID in soc_camera.h, and all
> code will use:
>     v4l2_clk_get(&client->dev, SOC_CAMERA_HOST_CLK_ID);
> 
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >   drivers/media/platform/soc_camera/soc_camera.c |  6 +++---
> >   drivers/media/usb/em28xx/em28xx-camera.c       |  2 +-
> >   drivers/media/v4l2-core/v4l2-clk.c             | 24
> > +++++++++++-------------
> >   include/media/v4l2-clk.h                       |  7 +++----
> >   4 files changed, 18 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/media/platform/soc_camera/soc_camera.c
> > b/drivers/media/platform/soc_camera/soc_camera.c
> > index f4be2a1..ce192b6 100644
> > --- a/drivers/media/platform/soc_camera/soc_camera.c
> > +++ b/drivers/media/platform/soc_camera/soc_camera.c
> > @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct
> > soc_camera_device *icd,
> >   	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
> >   		 shd->i2c_adapter_id, shd->board_info->addr);
> >   -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk",
> > icd);
> > +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
> >   	if (IS_ERR(icd->clk)) {
> >   		ret = PTR_ERR(icd->clk);
> >   		goto eclkreg;
> > @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host
> > *ici,
> >   	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
> >   		 sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address);
> >   -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk",
> > icd);
> > +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
> >   	if (IS_ERR(icd->clk)) {
> >   		ret = PTR_ERR(icd->clk);
> >   		goto eclkreg;
> > @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
> >   		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);
> > +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
> >   	if (IS_ERR(icd->clk)) {
> >   		ret = PTR_ERR(icd->clk);
> >   		goto eclkreg;
> > diff --git a/drivers/media/usb/em28xx/em28xx-camera.c
> > b/drivers/media/usb/em28xx/em28xx-camera.c
> > index 7be661f..a4b22c2 100644
> > --- a/drivers/media/usb/em28xx/em28xx-camera.c
> > +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> > @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev)
> >     	v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
> >   			  i2c_adapter_id(adap), client->addr);
> > -	v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL);
> > +	v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL);
> >   	if (IS_ERR(v4l2->clk))
> >   		return PTR_ERR(v4l2->clk);
> >   diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> > b/drivers/media/v4l2-core/v4l2-clk.c
> > index e18cc04..c210906 100644
> > --- a/drivers/media/v4l2-core/v4l2-clk.c
> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> > @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id,
> > const char *id)
> >   		if (strcmp(dev_id, clk->dev_id))
> >   			continue;
> >   -		if (!id || !clk->id || !strcmp(clk->id, id))
> > +		if ((!id && !clk->id) ||
> > +		    (id && clk->id && !strcmp(clk->id, id)))
> >   			return clk;
> >   	}
> >   @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
> >   	mutex_lock(&clk->lock);
> >     	enable = --clk->enable;
> > -	if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__,
> > -		 clk->dev_id, clk->id))
> > +	if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__,
> > +		 clk->dev_id))
> >   		clk->enable++;
> >   	else if (!enable && clk->ops->disable)
> >   		clk->ops->disable(clk);
> > @@ -181,7 +182,7 @@ EXPORT_SYMBOL(v4l2_clk_set_rate);
> >     struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
> >   				   const char *dev_id,
> > -				   const char *id, void *priv)
> > +				   void *priv)
> >   {
> >   	struct v4l2_clk *clk;
> >   	int ret;
> > @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct
> > v4l2_clk_ops *ops,
> >   	if (!clk)
> >   		return ERR_PTR(-ENOMEM);
> >   -	clk->id = kstrdup(id, GFP_KERNEL);
> >   	clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
> > -	if ((id && !clk->id) || !clk->dev_id) {
> > +	if (!clk->dev_id) {
> >   		ret = -ENOMEM;
> >   		goto ealloc;
> >   	}
> > @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct
> > v4l2_clk_ops *ops,
> >   	mutex_init(&clk->lock);
> >     	mutex_lock(&clk_lock);
> > -	if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
> > +	if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) {
> So the camera sensor driver should request the v4l2 clock with id is NULL?
> 
> How about define a SOC_CAMERA_HOST_CLK_ID in soc_camera.h, like:
>     #define SOC_CAMERA_HOST_CLK_ID    NULL
> 
> then we can rewrite this block code to:
> 
>         clk->ops = ops;
>         clk->priv = priv;
> +      clk->id = SOC_CAMERA_HOST_CLK_ID;
>         atomic_set(&clk->use_count, 0);
>         mutex_init(&clk->lock);
> 
>         mutex_lock(&clk_lock);
>         if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
>         ... ...
> 
> Best Regards,
> Josh Wu
> 
> >   		mutex_unlock(&clk_lock);
> >   		ret = -EEXIST;
> >   		goto eexist;
> > @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct
> > v4l2_clk_ops *ops,
> >     eexist:
> >   ealloc:
> > -	kfree(clk->id);
> >   	kfree(clk->dev_id);
> >   	kfree(clk);
> >   	return ERR_PTR(ret);
> > @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register);
> >   void v4l2_clk_unregister(struct v4l2_clk *clk)
> >   {
> >   	if (WARN(atomic_read(&clk->use_count),
> > -		 "%s(): Refusing to unregister ref-counted %s:%s clock!\n",
> > -		 __func__, clk->dev_id, clk->id))
> > +		 "%s(): Refusing to unregister ref-counted %s clock!\n",
> > +		 __func__, clk->dev_id))
> >   		return;
> >     	mutex_lock(&clk_lock);
> >   	list_del(&clk->list);
> >   	mutex_unlock(&clk_lock);
> >   -	kfree(clk->id);
> >   	kfree(clk->dev_id);
> >   	kfree(clk);
> >   }
> > @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk
> > *clk)
> >   }
> >     struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
> > -		const char *id, unsigned long rate, struct module *owner)
> > +				unsigned long rate, struct module *owner)
> >   {
> >   	struct v4l2_clk *clk;
> >   	struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char
> > *dev_id,
> >   	priv->ops.get_rate = fixed_get_rate;
> >   	priv->ops.owner = owner;
> >   -	clk = v4l2_clk_register(&priv->ops, dev_id, id, priv);
> > +	clk = v4l2_clk_register(&priv->ops, dev_id, priv);
> >   	if (IS_ERR(clk))
> >   		kfree(priv);
> >   diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> > index 0b36cc1..8f06967 100644
> > --- a/include/media/v4l2-clk.h
> > +++ b/include/media/v4l2-clk.h
> > @@ -43,7 +43,7 @@ struct v4l2_clk_ops {
> >     struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
> >   				   const char *dev_name,
> > -				   const char *name, void *priv);
> > +				   void *priv);
> >   void v4l2_clk_unregister(struct v4l2_clk *clk);
> >   struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id);
> >   void v4l2_clk_put(struct v4l2_clk *clk);
> > @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned
> > long rate);
> >   struct module;
> >     struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
> > -		const char *id, unsigned long rate, struct module *owner);
> > +			unsigned long rate, struct module *owner);
> >   void v4l2_clk_unregister_fixed(struct v4l2_clk *clk);
> >     static inline struct v4l2_clk *v4l2_clk_register_fixed(const char
> > *dev_id,
> > -							const char *id,
> >   							unsigned long rate)
> >   {
> > -	return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE);
> > +	return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE);
> >   }
> >     #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size,
> > \
> 

Re: [PATCH v2 2/2] V4L2: add CCF support to the v4l2_clk API

[Guennadi Liakhovetski]

On Mon, 5 Jan 2015, Josh Wu wrote:

> Hi, Guennadi
> 
> On 1/3/2015 4:18 AM, Guennadi Liakhovetski wrote:
> >  From aeaee56e04d023f3a019d2595ef5128015acdb06 Mon Sep 17 00:00:00 2001
> > From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Date: Fri, 2 Jan 2015 12:26:41 +0100
> > Subject: [PATCH 2/2] V4L2: add CCF support to the v4l2_clk API
> > 
> > V4L2 clocks, e.g. used by camera sensors for their master clock, do not
> > have to be supplied by a different V4L2 driver, they can also be
> > supplied by an independent source. In this case the standart kernel
> > clock API should be used to handle such clocks. This patch adds support
> > for such cases.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > 
> > Hi Laurent,
> > Thanks for the comment. The idea of allocating a new object for each "get"
> > operation seems a bit weird to me, and completely trusting the user is a
> > bit scary... :) But yes, it can work this way too, I think, and the user
> > can screw either way too, anyway. So, here comes a v2. Something like
> > this?
> > 
> > v2: don't add CCF-related clocks on the global list, just allocate a new
> > instance on each v4l2_clk_get()
> > 
> >   drivers/media/v4l2-core/v4l2-clk.c | 45
> > +++++++++++++++++++++++++++++++++++---
> >   include/media/v4l2-clk.h           |  2 ++
> >   2 files changed, 44 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> > b/drivers/media/v4l2-core/v4l2-clk.c
> > index c210906..f5d1688 100644
> > --- a/drivers/media/v4l2-core/v4l2-clk.c
> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> > @@ -9,6 +9,7 @@
> >    */
> >     #include <linux/atomic.h>
> > +#include <linux/clk.h>
> >   #include <linux/device.h>
> >   #include <linux/errno.h>
> >   #include <linux/list.h>
> > @@ -42,6 +43,18 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id,
> > const char *id)
> >   struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id)
> >   {
> >   	struct v4l2_clk *clk;
> > +	struct clk *ccf_clk = clk_get(dev, id);
> We need to check whether the 'id' is NULL, otherwise there is no error
> reported if we use IS_ERR(ccf_clk).

? Sorry, don't understand. AFAIK, id = NULL is absolutely valid, no need 
to check for it explicitly. However, it just occurs to me, that we 
probably need to change the below test

> Best Regards,
> Josh Wu
> 
> > +
> > +	if (!IS_ERR(ccf_clk)) {

to

> > +	if (!IS_ERR_OR_NULL(ccf_clk)) {

to cover the case when CCF isn't built at all and a stub is used. In that 
case it's probably better to fall back to the V4L2 clock emulation.

Thanks
Guennadi

> > +		clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
> > +		if (!clk) {
> > +			clk_put(ccf_clk);
> > +			return ERR_PTR(-ENOMEM);
> > +		}
> > +		clk->clk = ccf_clk;
> > +
> > +		return clk;
> > +	}
> >     	mutex_lock(&clk_lock);
> >   	clk = v4l2_clk_find(dev_name(dev), id);
> > @@ -61,6 +74,12 @@ void v4l2_clk_put(struct v4l2_clk *clk)
> >   	if (IS_ERR(clk))
> >   		return;
> >   +	if (clk->clk) {
> > +		clk_put(clk->clk);
> > +		kfree(clk);
> > +		return;
> > +	}
> > +
> >   	mutex_lock(&clk_lock);
> >     	list_for_each_entry(tmp, &clk_list, list)
> > @@ -98,8 +117,12 @@ static void v4l2_clk_unlock_driver(struct v4l2_clk *clk)
> >     int v4l2_clk_enable(struct v4l2_clk *clk)
> >   {
> > -	int ret = v4l2_clk_lock_driver(clk);
> > +	int ret;
> >   +	if (clk->clk)
> > +		return clk_enable(clk->clk);
> > +
> > +	ret = v4l2_clk_lock_driver(clk);
> >   	if (ret < 0)
> >   		return ret;
> >   @@ -125,6 +148,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
> >   {
> >   	int enable;
> >   +	if (clk->clk)
> > +		return clk_disable(clk->clk);
> > +
> >   	mutex_lock(&clk->lock);
> >     	enable = --clk->enable;
> > @@ -142,8 +168,12 @@ EXPORT_SYMBOL(v4l2_clk_disable);
> >     unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
> >   {
> > -	int ret = v4l2_clk_lock_driver(clk);
> > +	int ret;
> > +
> > +	if (clk->clk)
> > +		return clk_get_rate(clk->clk);
> >   +	ret = v4l2_clk_lock_driver(clk);
> >   	if (ret < 0)
> >   		return ret;
> >   @@ -162,7 +192,16 @@ EXPORT_SYMBOL(v4l2_clk_get_rate);
> >     int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
> >   {
> > -	int ret = v4l2_clk_lock_driver(clk);
> > +	int ret;
> > +
> > +	if (clk->clk) {
> > +		long r = clk_round_rate(clk->clk, rate);
> > +		if (r < 0)
> > +			return r;
> > +		return clk_set_rate(clk->clk, r);
> > +	}
> > +
> > +	ret = v4l2_clk_lock_driver(clk);
> >     	if (ret < 0)
> >   		return ret;
> > diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> > index 8f06967..4402b2d 100644
> > --- a/include/media/v4l2-clk.h
> > +++ b/include/media/v4l2-clk.h
> > @@ -22,6 +22,7 @@
> >   struct module;
> >   struct device;
> >   +struct clk;
> >   struct v4l2_clk {
> >   	struct list_head list;
> >   	const struct v4l2_clk_ops *ops;
> > @@ -30,6 +31,7 @@ struct v4l2_clk {
> >   	int enable;
> >   	struct mutex lock; /* Protect the enable count */
> >   	atomic_t use_count;
> > +	struct clk *clk;
> >   	void *priv;
> >   };
> >   

Re: [PATCH v2 2/2] V4L2: add CCF support to the v4l2_clk API

[Josh Wu]

On 1/5/2015 5:36 PM, Guennadi Liakhovetski wrote:
> On Mon, 5 Jan 2015, Josh Wu wrote:
>
>> Hi, Guennadi
>>
>> On 1/3/2015 4:18 AM, Guennadi Liakhovetski wrote:
>>>   From aeaee56e04d023f3a019d2595ef5128015acdb06 Mon Sep 17 00:00:00 2001
>>> From: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>> Date: Fri, 2 Jan 2015 12:26:41 +0100
>>> Subject: [PATCH 2/2] V4L2: add CCF support to the v4l2_clk API
>>>
>>> V4L2 clocks, e.g. used by camera sensors for their master clock, do not
>>> have to be supplied by a different V4L2 driver, they can also be
>>> supplied by an independent source. In this case the standart kernel
>>> clock API should be used to handle such clocks. This patch adds support
>>> for such cases.
>>>
>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>> ---
>>>
>>> Hi Laurent,
>>> Thanks for the comment. The idea of allocating a new object for each "get"
>>> operation seems a bit weird to me, and completely trusting the user is a
>>> bit scary... :) But yes, it can work this way too, I think, and the user
>>> can screw either way too, anyway. So, here comes a v2. Something like
>>> this?
>>>
>>> v2: don't add CCF-related clocks on the global list, just allocate a new
>>> instance on each v4l2_clk_get()
>>>
>>>    drivers/media/v4l2-core/v4l2-clk.c | 45
>>> +++++++++++++++++++++++++++++++++++---
>>>    include/media/v4l2-clk.h           |  2 ++
>>>    2 files changed, 44 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-clk.c
>>> b/drivers/media/v4l2-core/v4l2-clk.c
>>> index c210906..f5d1688 100644
>>> --- a/drivers/media/v4l2-core/v4l2-clk.c
>>> +++ b/drivers/media/v4l2-core/v4l2-clk.c
>>> @@ -9,6 +9,7 @@
>>>     */
>>>      #include <linux/atomic.h>
>>> +#include <linux/clk.h>
>>>    #include <linux/device.h>
>>>    #include <linux/errno.h>
>>>    #include <linux/list.h>
>>> @@ -42,6 +43,18 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id,
>>> const char *id)
>>>    struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id)
>>>    {
>>>    	struct v4l2_clk *clk;
>>> +	struct clk *ccf_clk = clk_get(dev, id);
>> We need to check whether the 'id' is NULL, otherwise there is no error
>> reported if we use IS_ERR(ccf_clk).
> ? Sorry, don't understand. AFAIK, id = NULL is absolutely valid, no need
> to check for it explicitly. However, it just occurs to me, that we
> probably need to change the below test
>
>> Best Regards,
>> Josh Wu
>>
>>> +
>>> +	if (!IS_ERR(ccf_clk)) {
> to
>
>>> +	if (!IS_ERR_OR_NULL(ccf_clk)) {
> to cover the case when CCF isn't built at all and a stub is used. In that
> case it's probably better to fall back to the V4L2 clock emulation.
yes, this's what I mean. but I don't know the function 
IS_ERR_OR_NULL().  ;-)

Best Regards,
Josh Wu

>
> Thanks
> Guennadi
>
>>> +		clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);
>>> +		if (!clk) {
>>> +			clk_put(ccf_clk);
>>> +			return ERR_PTR(-ENOMEM);
>>> +		}
>>> +		clk->clk = ccf_clk;
>>> +
>>> +		return clk;
>>> +	}
>>>      	mutex_lock(&clk_lock);
>>>    	clk = v4l2_clk_find(dev_name(dev), id);
>>> @@ -61,6 +74,12 @@ void v4l2_clk_put(struct v4l2_clk *clk)
>>>    	if (IS_ERR(clk))
>>>    		return;
>>>    +	if (clk->clk) {
>>> +		clk_put(clk->clk);
>>> +		kfree(clk);
>>> +		return;
>>> +	}
>>> +
>>>    	mutex_lock(&clk_lock);
>>>      	list_for_each_entry(tmp, &clk_list, list)
>>> @@ -98,8 +117,12 @@ static void v4l2_clk_unlock_driver(struct v4l2_clk *clk)
>>>      int v4l2_clk_enable(struct v4l2_clk *clk)
>>>    {
>>> -	int ret = v4l2_clk_lock_driver(clk);
>>> +	int ret;
>>>    +	if (clk->clk)
>>> +		return clk_enable(clk->clk);
>>> +
>>> +	ret = v4l2_clk_lock_driver(clk);
>>>    	if (ret < 0)
>>>    		return ret;
>>>    @@ -125,6 +148,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
>>>    {
>>>    	int enable;
>>>    +	if (clk->clk)
>>> +		return clk_disable(clk->clk);
>>> +
>>>    	mutex_lock(&clk->lock);
>>>      	enable = --clk->enable;
>>> @@ -142,8 +168,12 @@ EXPORT_SYMBOL(v4l2_clk_disable);
>>>      unsigned long v4l2_clk_get_rate(struct v4l2_clk *clk)
>>>    {
>>> -	int ret = v4l2_clk_lock_driver(clk);
>>> +	int ret;
>>> +
>>> +	if (clk->clk)
>>> +		return clk_get_rate(clk->clk);
>>>    +	ret = v4l2_clk_lock_driver(clk);
>>>    	if (ret < 0)
>>>    		return ret;
>>>    @@ -162,7 +192,16 @@ EXPORT_SYMBOL(v4l2_clk_get_rate);
>>>      int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate)
>>>    {
>>> -	int ret = v4l2_clk_lock_driver(clk);
>>> +	int ret;
>>> +
>>> +	if (clk->clk) {
>>> +		long r = clk_round_rate(clk->clk, rate);
>>> +		if (r < 0)
>>> +			return r;
>>> +		return clk_set_rate(clk->clk, r);
>>> +	}
>>> +
>>> +	ret = v4l2_clk_lock_driver(clk);
>>>      	if (ret < 0)
>>>    		return ret;
>>> diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
>>> index 8f06967..4402b2d 100644
>>> --- a/include/media/v4l2-clk.h
>>> +++ b/include/media/v4l2-clk.h
>>> @@ -22,6 +22,7 @@
>>>    struct module;
>>>    struct device;
>>>    +struct clk;
>>>    struct v4l2_clk {
>>>    	struct list_head list;
>>>    	const struct v4l2_clk_ops *ops;
>>> @@ -30,6 +31,7 @@ struct v4l2_clk {
>>>    	int enable;
>>>    	struct mutex lock; /* Protect the enable count */
>>>    	atomic_t use_count;
>>> +	struct clk *clk;
>>>    	void *priv;
>>>    };
>>>    

Re: [PATCH 1/2] V4L: remove clock name from v4l2_clk API

[Josh Wu]

Hi, Guennadi

On 1/5/2015 5:28 PM, Guennadi Liakhovetski wrote:
> Hi Josh,
>
> On Mon, 5 Jan 2015, Josh Wu wrote:
>
>> Hi, Guennadi
>>
>> On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote:
>>> All uses of the v4l2_clk API so far only register one clock with a fixed
>>> name. This allows us to get rid of it, which also will make CCF and DT
>>> integration easier.
>> This patch not register a v4l2_clk with name: "mclk". So this will break all
>> the i2c sensors that used the v4l2 clock "mclk".
>> To make those drivers work, we need change all lines from:
>>     v4l2_clk_get(&client->dev, "mclk");
>> to:
>>     v4l2_clk_get(&client->dev, NULL);
>>
>> Am I understanding correctly?
> No, it's the opposite. Clock consumers should request clock names,
> according to their datasheets. In ov2640 case it's xvclk. For v4l2_clk the
> name will simply be ignored. But it will be used to try to get a CCF clock
> before falling back to V4L2 clocks.
Okay, you mean I only need request xvclk clock via call v4l2_clk_get() 
in ov2640 driver. right?
So the question is how do we operation the camera host's 
.clock_start/stop()? Which is by v4l2_clk_get() old "mclk".

hmm, wait. Since this part is not belong to ov2640, I think we should 
move this part code to soc_camera.c 's function: soc_camera_power_on().
or do I miss anything?

>
> Thanks
> Guennadi

Best Regards,
Josh Wu
>> if yes, I'd like to define a SOC_CAMERA_HOST_CLK_ID in soc_camera.h, and all
>> code will use:
>>      v4l2_clk_get(&client->dev, SOC_CAMERA_HOST_CLK_ID);
>>
>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>> ---
>>>    drivers/media/platform/soc_camera/soc_camera.c |  6 +++---
>>>    drivers/media/usb/em28xx/em28xx-camera.c       |  2 +-
>>>    drivers/media/v4l2-core/v4l2-clk.c             | 24
>>> +++++++++++-------------
>>>    include/media/v4l2-clk.h                       |  7 +++----
>>>    4 files changed, 18 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c
>>> b/drivers/media/platform/soc_camera/soc_camera.c
>>> index f4be2a1..ce192b6 100644
>>> --- a/drivers/media/platform/soc_camera/soc_camera.c
>>> +++ b/drivers/media/platform/soc_camera/soc_camera.c
>>> @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct
>>> soc_camera_device *icd,
>>>    	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
>>>    		 shd->i2c_adapter_id, shd->board_info->addr);
>>>    -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk",
>>> icd);
>>> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>>>    	if (IS_ERR(icd->clk)) {
>>>    		ret = PTR_ERR(icd->clk);
>>>    		goto eclkreg;
>>> @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host
>>> *ici,
>>>    	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
>>>    		 sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address);
>>>    -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk",
>>> icd);
>>> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>>>    	if (IS_ERR(icd->clk)) {
>>>    		ret = PTR_ERR(icd->clk);
>>>    		goto eclkreg;
>>> @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
>>>    		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);
>>> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>>>    	if (IS_ERR(icd->clk)) {
>>>    		ret = PTR_ERR(icd->clk);
>>>    		goto eclkreg;
>>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c
>>> b/drivers/media/usb/em28xx/em28xx-camera.c
>>> index 7be661f..a4b22c2 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-camera.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
>>> @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev)
>>>      	v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
>>>    			  i2c_adapter_id(adap), client->addr);
>>> -	v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL);
>>> +	v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL);
>>>    	if (IS_ERR(v4l2->clk))
>>>    		return PTR_ERR(v4l2->clk);
>>>    diff --git a/drivers/media/v4l2-core/v4l2-clk.c
>>> b/drivers/media/v4l2-core/v4l2-clk.c
>>> index e18cc04..c210906 100644
>>> --- a/drivers/media/v4l2-core/v4l2-clk.c
>>> +++ b/drivers/media/v4l2-core/v4l2-clk.c
>>> @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id,
>>> const char *id)
>>>    		if (strcmp(dev_id, clk->dev_id))
>>>    			continue;
>>>    -		if (!id || !clk->id || !strcmp(clk->id, id))
>>> +		if ((!id && !clk->id) ||
>>> +		    (id && clk->id && !strcmp(clk->id, id)))
>>>    			return clk;
>>>    	}
>>>    @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
>>>    	mutex_lock(&clk->lock);
>>>      	enable = --clk->enable;
>>> -	if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__,
>>> -		 clk->dev_id, clk->id))
>>> +	if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__,
>>> +		 clk->dev_id))
>>>    		clk->enable++;
>>>    	else if (!enable && clk->ops->disable)
>>>    		clk->ops->disable(clk);
>>> @@ -181,7 +182,7 @@ EXPORT_SYMBOL(v4l2_clk_set_rate);
>>>      struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>>>    				   const char *dev_id,
>>> -				   const char *id, void *priv)
>>> +				   void *priv)
>>>    {
>>>    	struct v4l2_clk *clk;
>>>    	int ret;
>>> @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct
>>> v4l2_clk_ops *ops,
>>>    	if (!clk)
>>>    		return ERR_PTR(-ENOMEM);
>>>    -	clk->id = kstrdup(id, GFP_KERNEL);
>>>    	clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
>>> -	if ((id && !clk->id) || !clk->dev_id) {
>>> +	if (!clk->dev_id) {
>>>    		ret = -ENOMEM;
>>>    		goto ealloc;
>>>    	}
>>> @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct
>>> v4l2_clk_ops *ops,
>>>    	mutex_init(&clk->lock);
>>>      	mutex_lock(&clk_lock);
>>> -	if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
>>> +	if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) {
>> So the camera sensor driver should request the v4l2 clock with id is NULL?
>>
>> How about define a SOC_CAMERA_HOST_CLK_ID in soc_camera.h, like:
>>      #define SOC_CAMERA_HOST_CLK_ID    NULL
>>
>> then we can rewrite this block code to:
>>
>>          clk->ops = ops;
>>          clk->priv = priv;
>> +      clk->id = SOC_CAMERA_HOST_CLK_ID;
>>          atomic_set(&clk->use_count, 0);
>>          mutex_init(&clk->lock);
>>
>>          mutex_lock(&clk_lock);
>>          if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
>>          ... ...
>>
>> Best Regards,
>> Josh Wu
>>
>>>    		mutex_unlock(&clk_lock);
>>>    		ret = -EEXIST;
>>>    		goto eexist;
>>> @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct
>>> v4l2_clk_ops *ops,
>>>      eexist:
>>>    ealloc:
>>> -	kfree(clk->id);
>>>    	kfree(clk->dev_id);
>>>    	kfree(clk);
>>>    	return ERR_PTR(ret);
>>> @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register);
>>>    void v4l2_clk_unregister(struct v4l2_clk *clk)
>>>    {
>>>    	if (WARN(atomic_read(&clk->use_count),
>>> -		 "%s(): Refusing to unregister ref-counted %s:%s clock!\n",
>>> -		 __func__, clk->dev_id, clk->id))
>>> +		 "%s(): Refusing to unregister ref-counted %s clock!\n",
>>> +		 __func__, clk->dev_id))
>>>    		return;
>>>      	mutex_lock(&clk_lock);
>>>    	list_del(&clk->list);
>>>    	mutex_unlock(&clk_lock);
>>>    -	kfree(clk->id);
>>>    	kfree(clk->dev_id);
>>>    	kfree(clk);
>>>    }
>>> @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk
>>> *clk)
>>>    }
>>>      struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
>>> -		const char *id, unsigned long rate, struct module *owner)
>>> +				unsigned long rate, struct module *owner)
>>>    {
>>>    	struct v4l2_clk *clk;
>>>    	struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>>> @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char
>>> *dev_id,
>>>    	priv->ops.get_rate = fixed_get_rate;
>>>    	priv->ops.owner = owner;
>>>    -	clk = v4l2_clk_register(&priv->ops, dev_id, id, priv);
>>> +	clk = v4l2_clk_register(&priv->ops, dev_id, priv);
>>>    	if (IS_ERR(clk))
>>>    		kfree(priv);
>>>    diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
>>> index 0b36cc1..8f06967 100644
>>> --- a/include/media/v4l2-clk.h
>>> +++ b/include/media/v4l2-clk.h
>>> @@ -43,7 +43,7 @@ struct v4l2_clk_ops {
>>>      struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>>>    				   const char *dev_name,
>>> -				   const char *name, void *priv);
>>> +				   void *priv);
>>>    void v4l2_clk_unregister(struct v4l2_clk *clk);
>>>    struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id);
>>>    void v4l2_clk_put(struct v4l2_clk *clk);
>>> @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned
>>> long rate);
>>>    struct module;
>>>      struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
>>> -		const char *id, unsigned long rate, struct module *owner);
>>> +			unsigned long rate, struct module *owner);
>>>    void v4l2_clk_unregister_fixed(struct v4l2_clk *clk);
>>>      static inline struct v4l2_clk *v4l2_clk_register_fixed(const char
>>> *dev_id,
>>> -							const char *id,
>>>    							unsigned long rate)
>>>    {
>>> -	return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE);
>>> +	return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE);
>>>    }
>>>      #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size,
>>> \

Re: [PATCH 1/2] V4L: remove clock name from v4l2_clk API

[Josh Wu]

Hi, Guennadi

After look deep into this patch, I found you miss one line that should 
be changed as well.
It's In function v4l2_clk_get(), there still has one line code called 
v4l2_clk_find(dev_id, id).
You need to change it to v4l2_clk_find(dev_id, NULL) as well.
Otherwise the code that many sensor used: v4l2_clk_get(&client->dev, 
"mclk") cannot acquired the "mclk" clock.

After above changes, this patch works for me.

On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote:
> All uses of the v4l2_clk API so far only register one clock with a fixed
> name. This allows us to get rid of it, which also will make CCF and DT
> integration easier.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>   drivers/media/platform/soc_camera/soc_camera.c |  6 +++---
>   drivers/media/usb/em28xx/em28xx-camera.c       |  2 +-
>   drivers/media/v4l2-core/v4l2-clk.c             | 24 +++++++++++-------------
>   include/media/v4l2-clk.h                       |  7 +++----
>   4 files changed, 18 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index f4be2a1..ce192b6 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct soc_camera_device *icd,
>   	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
>   		 shd->i2c_adapter_id, shd->board_info->addr);
>   
> -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd);
> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>   	if (IS_ERR(icd->clk)) {
>   		ret = PTR_ERR(icd->clk);
>   		goto eclkreg;
> @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host *ici,
>   	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
>   		 sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address);
>   
> -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk", icd);
> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>   	if (IS_ERR(icd->clk)) {
>   		ret = PTR_ERR(icd->clk);
>   		goto eclkreg;
> @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
>   		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);
> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>   	if (IS_ERR(icd->clk)) {
>   		ret = PTR_ERR(icd->clk);
>   		goto eclkreg;
> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c b/drivers/media/usb/em28xx/em28xx-camera.c
> index 7be661f..a4b22c2 100644
> --- a/drivers/media/usb/em28xx/em28xx-camera.c
> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev)
>   
>   	v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
>   			  i2c_adapter_id(adap), client->addr);
> -	v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL);
> +	v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL);
>   	if (IS_ERR(v4l2->clk))
>   		return PTR_ERR(v4l2->clk);
>   
> diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> index e18cc04..c210906 100644
> --- a/drivers/media/v4l2-core/v4l2-clk.c
> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id)
>   		if (strcmp(dev_id, clk->dev_id))
>   			continue;
>   
> -		if (!id || !clk->id || !strcmp(clk->id, id))
> +		if ((!id && !clk->id) ||
> +		    (id && clk->id && !strcmp(clk->id, id)))
>   			return clk;
>   	}
>   
> @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
>   	mutex_lock(&clk->lock);
>   
>   	enable = --clk->enable;
> -	if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__,
> -		 clk->dev_id, clk->id))
> +	if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__,
> +		 clk->dev_id))
>   		clk->enable++;
>   	else if (!enable && clk->ops->disable)
>   		clk->ops->disable(clk);
> @@ -181,7 +182,7 @@ EXPORT_SYMBOL(v4l2_clk_set_rate);
>   
>   struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>   				   const char *dev_id,
> -				   const char *id, void *priv)
> +				   void *priv)
>   {
>   	struct v4l2_clk *clk;
>   	int ret;
> @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>   	if (!clk)
>   		return ERR_PTR(-ENOMEM);
>   
> -	clk->id = kstrdup(id, GFP_KERNEL);
>   	clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
> -	if ((id && !clk->id) || !clk->dev_id) {
> +	if (!clk->dev_id) {
>   		ret = -ENOMEM;
>   		goto ealloc;
>   	}
> @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>   	mutex_init(&clk->lock);
>   
>   	mutex_lock(&clk_lock);
> -	if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
> +	if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) {

like mentioned in the beginning of the mail, you need to do same change 
in v4l2_clk_get().

Best Regards,
Josh Wu


>   		mutex_unlock(&clk_lock);
>   		ret = -EEXIST;
>   		goto eexist;
> @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>   
>   eexist:
>   ealloc:
> -	kfree(clk->id);
>   	kfree(clk->dev_id);
>   	kfree(clk);
>   	return ERR_PTR(ret);
> @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register);
>   void v4l2_clk_unregister(struct v4l2_clk *clk)
>   {
>   	if (WARN(atomic_read(&clk->use_count),
> -		 "%s(): Refusing to unregister ref-counted %s:%s clock!\n",
> -		 __func__, clk->dev_id, clk->id))
> +		 "%s(): Refusing to unregister ref-counted %s clock!\n",
> +		 __func__, clk->dev_id))
>   		return;
>   
>   	mutex_lock(&clk_lock);
>   	list_del(&clk->list);
>   	mutex_unlock(&clk_lock);
>   
> -	kfree(clk->id);
>   	kfree(clk->dev_id);
>   	kfree(clk);
>   }
> @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk *clk)
>   }
>   
>   struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
> -		const char *id, unsigned long rate, struct module *owner)
> +				unsigned long rate, struct module *owner)
>   {
>   	struct v4l2_clk *clk;
>   	struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
>   	priv->ops.get_rate = fixed_get_rate;
>   	priv->ops.owner = owner;
>   
> -	clk = v4l2_clk_register(&priv->ops, dev_id, id, priv);
> +	clk = v4l2_clk_register(&priv->ops, dev_id, priv);
>   	if (IS_ERR(clk))
>   		kfree(priv);
>   
> diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> index 0b36cc1..8f06967 100644
> --- a/include/media/v4l2-clk.h
> +++ b/include/media/v4l2-clk.h
> @@ -43,7 +43,7 @@ struct v4l2_clk_ops {
>   
>   struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>   				   const char *dev_name,
> -				   const char *name, void *priv);
> +				   void *priv);
>   void v4l2_clk_unregister(struct v4l2_clk *clk);
>   struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id);
>   void v4l2_clk_put(struct v4l2_clk *clk);
> @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned long rate);
>   struct module;
>   
>   struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
> -		const char *id, unsigned long rate, struct module *owner);
> +			unsigned long rate, struct module *owner);
>   void v4l2_clk_unregister_fixed(struct v4l2_clk *clk);
>   
>   static inline struct v4l2_clk *v4l2_clk_register_fixed(const char *dev_id,
> -							const char *id,
>   							unsigned long rate)
>   {
> -	return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE);
> +	return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE);
>   }
>   
>   #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size, \

Re: [PATCH 1/2] V4L: remove clock name from v4l2_clk API

[Guennadi Liakhovetski]

Hi Josh,

On Tue, 6 Jan 2015, Josh Wu wrote:

> Hi, Guennadi
> 
> After look deep into this patch, I found you miss one line that should be
> changed as well.
> It's In function v4l2_clk_get(), there still has one line code called
> v4l2_clk_find(dev_id, id).
> You need to change it to v4l2_clk_find(dev_id, NULL) as well.
> Otherwise the code that many sensor used: v4l2_clk_get(&client->dev, "mclk")
> cannot acquired the "mclk" clock.
> 
> After above changes, this patch works for me.

I think you're right, in fact, since we now don't store CCF-based v4l2_clk 
wrappers on the list, this can be simplified even further, I'll update the 
patch. Did you only test this patch or both?

Thanks
Guennadi

> 
> On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote:
> > All uses of the v4l2_clk API so far only register one clock with a fixed
> > name. This allows us to get rid of it, which also will make CCF and DT
> > integration easier.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >   drivers/media/platform/soc_camera/soc_camera.c |  6 +++---
> >   drivers/media/usb/em28xx/em28xx-camera.c       |  2 +-
> >   drivers/media/v4l2-core/v4l2-clk.c             | 24
> > +++++++++++-------------
> >   include/media/v4l2-clk.h                       |  7 +++----
> >   4 files changed, 18 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/media/platform/soc_camera/soc_camera.c
> > b/drivers/media/platform/soc_camera/soc_camera.c
> > index f4be2a1..ce192b6 100644
> > --- a/drivers/media/platform/soc_camera/soc_camera.c
> > +++ b/drivers/media/platform/soc_camera/soc_camera.c
> > @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct
> > soc_camera_device *icd,
> >   	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
> >   		 shd->i2c_adapter_id, shd->board_info->addr);
> >   -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk",
> > icd);
> > +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
> >   	if (IS_ERR(icd->clk)) {
> >   		ret = PTR_ERR(icd->clk);
> >   		goto eclkreg;
> > @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host
> > *ici,
> >   	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
> >   		 sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address);
> >   -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk",
> > icd);
> > +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
> >   	if (IS_ERR(icd->clk)) {
> >   		ret = PTR_ERR(icd->clk);
> >   		goto eclkreg;
> > @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
> >   		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);
> > +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
> >   	if (IS_ERR(icd->clk)) {
> >   		ret = PTR_ERR(icd->clk);
> >   		goto eclkreg;
> > diff --git a/drivers/media/usb/em28xx/em28xx-camera.c
> > b/drivers/media/usb/em28xx/em28xx-camera.c
> > index 7be661f..a4b22c2 100644
> > --- a/drivers/media/usb/em28xx/em28xx-camera.c
> > +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> > @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev)
> >     	v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
> >   			  i2c_adapter_id(adap), client->addr);
> > -	v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL);
> > +	v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL);
> >   	if (IS_ERR(v4l2->clk))
> >   		return PTR_ERR(v4l2->clk);
> >   diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> > b/drivers/media/v4l2-core/v4l2-clk.c
> > index e18cc04..c210906 100644
> > --- a/drivers/media/v4l2-core/v4l2-clk.c
> > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> > @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id,
> > const char *id)
> >   		if (strcmp(dev_id, clk->dev_id))
> >   			continue;
> >   -		if (!id || !clk->id || !strcmp(clk->id, id))
> > +		if ((!id && !clk->id) ||
> > +		    (id && clk->id && !strcmp(clk->id, id)))
> >   			return clk;
> >   	}
> >   @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
> >   	mutex_lock(&clk->lock);
> >     	enable = --clk->enable;
> > -	if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__,
> > -		 clk->dev_id, clk->id))
> > +	if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__,
> > +		 clk->dev_id))
> >   		clk->enable++;
> >   	else if (!enable && clk->ops->disable)
> >   		clk->ops->disable(clk);
> > @@ -181,7 +182,7 @@ EXPORT_SYMBOL(v4l2_clk_set_rate);
> >     struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
> >   				   const char *dev_id,
> > -				   const char *id, void *priv)
> > +				   void *priv)
> >   {
> >   	struct v4l2_clk *clk;
> >   	int ret;
> > @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct
> > v4l2_clk_ops *ops,
> >   	if (!clk)
> >   		return ERR_PTR(-ENOMEM);
> >   -	clk->id = kstrdup(id, GFP_KERNEL);
> >   	clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
> > -	if ((id && !clk->id) || !clk->dev_id) {
> > +	if (!clk->dev_id) {
> >   		ret = -ENOMEM;
> >   		goto ealloc;
> >   	}
> > @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct
> > v4l2_clk_ops *ops,
> >   	mutex_init(&clk->lock);
> >     	mutex_lock(&clk_lock);
> > -	if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
> > +	if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) {
> 
> like mentioned in the beginning of the mail, you need to do same change in
> v4l2_clk_get().
> 
> Best Regards,
> Josh Wu
> 
> 
> >   		mutex_unlock(&clk_lock);
> >   		ret = -EEXIST;
> >   		goto eexist;
> > @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct
> > v4l2_clk_ops *ops,
> >     eexist:
> >   ealloc:
> > -	kfree(clk->id);
> >   	kfree(clk->dev_id);
> >   	kfree(clk);
> >   	return ERR_PTR(ret);
> > @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register);
> >   void v4l2_clk_unregister(struct v4l2_clk *clk)
> >   {
> >   	if (WARN(atomic_read(&clk->use_count),
> > -		 "%s(): Refusing to unregister ref-counted %s:%s clock!\n",
> > -		 __func__, clk->dev_id, clk->id))
> > +		 "%s(): Refusing to unregister ref-counted %s clock!\n",
> > +		 __func__, clk->dev_id))
> >   		return;
> >     	mutex_lock(&clk_lock);
> >   	list_del(&clk->list);
> >   	mutex_unlock(&clk_lock);
> >   -	kfree(clk->id);
> >   	kfree(clk->dev_id);
> >   	kfree(clk);
> >   }
> > @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk
> > *clk)
> >   }
> >     struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
> > -		const char *id, unsigned long rate, struct module *owner)
> > +				unsigned long rate, struct module *owner)
> >   {
> >   	struct v4l2_clk *clk;
> >   	struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char
> > *dev_id,
> >   	priv->ops.get_rate = fixed_get_rate;
> >   	priv->ops.owner = owner;
> >   -	clk = v4l2_clk_register(&priv->ops, dev_id, id, priv);
> > +	clk = v4l2_clk_register(&priv->ops, dev_id, priv);
> >   	if (IS_ERR(clk))
> >   		kfree(priv);
> >   diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> > index 0b36cc1..8f06967 100644
> > --- a/include/media/v4l2-clk.h
> > +++ b/include/media/v4l2-clk.h
> > @@ -43,7 +43,7 @@ struct v4l2_clk_ops {
> >     struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
> >   				   const char *dev_name,
> > -				   const char *name, void *priv);
> > +				   void *priv);
> >   void v4l2_clk_unregister(struct v4l2_clk *clk);
> >   struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id);
> >   void v4l2_clk_put(struct v4l2_clk *clk);
> > @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned
> > long rate);
> >   struct module;
> >     struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
> > -		const char *id, unsigned long rate, struct module *owner);
> > +			unsigned long rate, struct module *owner);
> >   void v4l2_clk_unregister_fixed(struct v4l2_clk *clk);
> >     static inline struct v4l2_clk *v4l2_clk_register_fixed(const char
> > *dev_id,
> > -							const char *id,
> >   							unsigned long rate)
> >   {
> > -	return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE);
> > +	return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE);
> >   }
> >     #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size,
> > \
> 

Re: [PATCH 1/2] V4L: remove clock name from v4l2_clk API

[Josh Wu]

Hi, Guennadi

On 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote:
> Hi Josh,
>
> On Tue, 6 Jan 2015, Josh Wu wrote:
>
>> Hi, Guennadi
>>
>> After look deep into this patch, I found you miss one line that should be
>> changed as well.
>> It's In function v4l2_clk_get(), there still has one line code called
>> v4l2_clk_find(dev_id, id).
>> You need to change it to v4l2_clk_find(dev_id, NULL) as well.
>> Otherwise the code that many sensor used: v4l2_clk_get(&client->dev, "mclk")
>> cannot acquired the "mclk" clock.
>>
>> After above changes, this patch works for me.
> I think you're right, in fact, since we now don't store CCF-based v4l2_clk
> wrappers on the list, this can be simplified even further, I'll update the
> patch. Did you only test this patch or both?
I tested both patches with Atmel-isi driver. For the 2/2 patch I applied 
the modification Laurent suggested.
Those patches works for me.

The only concern is in ov2640 I still need to acquired two v4l2 clocks:
    "xvclk"  that will get the xvclk CCF clock directly.
    "mclk"  that make ISI driver call his clock_start()/stop() to 
enable/disable ISI's peripheral clock.
If I only get xvclk clock, then the camera capture will be failed with a 
ISI timeout error.

But I think this is acceptable as we will keep go forward. Finally we'll 
switch to CCF and removed the v4l2_clock then we will move the 
clock_start()/stop() caller code to soc_camera.c.

Best Regards,
Josh Wu

>
> Thanks
> Guennadi
>
>> On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote:
>>> All uses of the v4l2_clk API so far only register one clock with a fixed
>>> name. This allows us to get rid of it, which also will make CCF and DT
>>> integration easier.
>>>
>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>> ---
>>>    drivers/media/platform/soc_camera/soc_camera.c |  6 +++---
>>>    drivers/media/usb/em28xx/em28xx-camera.c       |  2 +-
>>>    drivers/media/v4l2-core/v4l2-clk.c             | 24
>>> +++++++++++-------------
>>>    include/media/v4l2-clk.h                       |  7 +++----
>>>    4 files changed, 18 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c
>>> b/drivers/media/platform/soc_camera/soc_camera.c
>>> index f4be2a1..ce192b6 100644
>>> --- a/drivers/media/platform/soc_camera/soc_camera.c
>>> +++ b/drivers/media/platform/soc_camera/soc_camera.c
>>> @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct
>>> soc_camera_device *icd,
>>>    	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
>>>    		 shd->i2c_adapter_id, shd->board_info->addr);
>>>    -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk",
>>> icd);
>>> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>>>    	if (IS_ERR(icd->clk)) {
>>>    		ret = PTR_ERR(icd->clk);
>>>    		goto eclkreg;
>>> @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host
>>> *ici,
>>>    	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
>>>    		 sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address);
>>>    -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk",
>>> icd);
>>> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>>>    	if (IS_ERR(icd->clk)) {
>>>    		ret = PTR_ERR(icd->clk);
>>>    		goto eclkreg;
>>> @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
>>>    		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);
>>> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>>>    	if (IS_ERR(icd->clk)) {
>>>    		ret = PTR_ERR(icd->clk);
>>>    		goto eclkreg;
>>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c
>>> b/drivers/media/usb/em28xx/em28xx-camera.c
>>> index 7be661f..a4b22c2 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-camera.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
>>> @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev)
>>>      	v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
>>>    			  i2c_adapter_id(adap), client->addr);
>>> -	v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL);
>>> +	v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL);
>>>    	if (IS_ERR(v4l2->clk))
>>>    		return PTR_ERR(v4l2->clk);
>>>    diff --git a/drivers/media/v4l2-core/v4l2-clk.c
>>> b/drivers/media/v4l2-core/v4l2-clk.c
>>> index e18cc04..c210906 100644
>>> --- a/drivers/media/v4l2-core/v4l2-clk.c
>>> +++ b/drivers/media/v4l2-core/v4l2-clk.c
>>> @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id,
>>> const char *id)
>>>    		if (strcmp(dev_id, clk->dev_id))
>>>    			continue;
>>>    -		if (!id || !clk->id || !strcmp(clk->id, id))
>>> +		if ((!id && !clk->id) ||
>>> +		    (id && clk->id && !strcmp(clk->id, id)))
>>>    			return clk;
>>>    	}
>>>    @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
>>>    	mutex_lock(&clk->lock);
>>>      	enable = --clk->enable;
>>> -	if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__,
>>> -		 clk->dev_id, clk->id))
>>> +	if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__,
>>> +		 clk->dev_id))
>>>    		clk->enable++;
>>>    	else if (!enable && clk->ops->disable)
>>>    		clk->ops->disable(clk);
>>> @@ -181,7 +182,7 @@ EXPORT_SYMBOL(v4l2_clk_set_rate);
>>>      struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>>>    				   const char *dev_id,
>>> -				   const char *id, void *priv)
>>> +				   void *priv)
>>>    {
>>>    	struct v4l2_clk *clk;
>>>    	int ret;
>>> @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct
>>> v4l2_clk_ops *ops,
>>>    	if (!clk)
>>>    		return ERR_PTR(-ENOMEM);
>>>    -	clk->id = kstrdup(id, GFP_KERNEL);
>>>    	clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
>>> -	if ((id && !clk->id) || !clk->dev_id) {
>>> +	if (!clk->dev_id) {
>>>    		ret = -ENOMEM;
>>>    		goto ealloc;
>>>    	}
>>> @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct
>>> v4l2_clk_ops *ops,
>>>    	mutex_init(&clk->lock);
>>>      	mutex_lock(&clk_lock);
>>> -	if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
>>> +	if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) {
>> like mentioned in the beginning of the mail, you need to do same change in
>> v4l2_clk_get().
>>
>> Best Regards,
>> Josh Wu
>>
>>
>>>    		mutex_unlock(&clk_lock);
>>>    		ret = -EEXIST;
>>>    		goto eexist;
>>> @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct
>>> v4l2_clk_ops *ops,
>>>      eexist:
>>>    ealloc:
>>> -	kfree(clk->id);
>>>    	kfree(clk->dev_id);
>>>    	kfree(clk);
>>>    	return ERR_PTR(ret);
>>> @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register);
>>>    void v4l2_clk_unregister(struct v4l2_clk *clk)
>>>    {
>>>    	if (WARN(atomic_read(&clk->use_count),
>>> -		 "%s(): Refusing to unregister ref-counted %s:%s clock!\n",
>>> -		 __func__, clk->dev_id, clk->id))
>>> +		 "%s(): Refusing to unregister ref-counted %s clock!\n",
>>> +		 __func__, clk->dev_id))
>>>    		return;
>>>      	mutex_lock(&clk_lock);
>>>    	list_del(&clk->list);
>>>    	mutex_unlock(&clk_lock);
>>>    -	kfree(clk->id);
>>>    	kfree(clk->dev_id);
>>>    	kfree(clk);
>>>    }
>>> @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk
>>> *clk)
>>>    }
>>>      struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
>>> -		const char *id, unsigned long rate, struct module *owner)
>>> +				unsigned long rate, struct module *owner)
>>>    {
>>>    	struct v4l2_clk *clk;
>>>    	struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>>> @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char
>>> *dev_id,
>>>    	priv->ops.get_rate = fixed_get_rate;
>>>    	priv->ops.owner = owner;
>>>    -	clk = v4l2_clk_register(&priv->ops, dev_id, id, priv);
>>> +	clk = v4l2_clk_register(&priv->ops, dev_id, priv);
>>>    	if (IS_ERR(clk))
>>>    		kfree(priv);
>>>    diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
>>> index 0b36cc1..8f06967 100644
>>> --- a/include/media/v4l2-clk.h
>>> +++ b/include/media/v4l2-clk.h
>>> @@ -43,7 +43,7 @@ struct v4l2_clk_ops {
>>>      struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>>>    				   const char *dev_name,
>>> -				   const char *name, void *priv);
>>> +				   void *priv);
>>>    void v4l2_clk_unregister(struct v4l2_clk *clk);
>>>    struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id);
>>>    void v4l2_clk_put(struct v4l2_clk *clk);
>>> @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned
>>> long rate);
>>>    struct module;
>>>      struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
>>> -		const char *id, unsigned long rate, struct module *owner);
>>> +			unsigned long rate, struct module *owner);
>>>    void v4l2_clk_unregister_fixed(struct v4l2_clk *clk);
>>>      static inline struct v4l2_clk *v4l2_clk_register_fixed(const char
>>> *dev_id,
>>> -							const char *id,
>>>    							unsigned long rate)
>>>    {
>>> -	return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE);
>>> +	return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE);
>>>    }
>>>      #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size,
>>> \

Re: [PATCH 1/2] V4L: remove clock name from v4l2_clk API

[Josh Wu]

Hi, Guennadi

On 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote:
> Hi Josh,
>
> On Tue, 6 Jan 2015, Josh Wu wrote:
>
>> Hi, Guennadi
>>
>> After look deep into this patch, I found you miss one line that should be
>> changed as well.
>> It's In function v4l2_clk_get(), there still has one line code called
>> v4l2_clk_find(dev_id, id).
>> You need to change it to v4l2_clk_find(dev_id, NULL) as well.
>> Otherwise the code that many sensor used: v4l2_clk_get(&client->dev, "mclk")
>> cannot acquired the "mclk" clock.
>>
>> After above changes, this patch works for me.
> I think you're right, in fact, since we now don't store CCF-based v4l2_clk
> wrappers on the list, this can be simplified even further, I'll update the
> patch. Did you only test this patch or both?
I tested both patches with Atmel-isi driver. For the 2/2 patch I applied 
the modification Laurent suggested.
Those patches works for me.

The only concern is in ov2640 I still need to acquired two v4l2 clocks:
    "xvclk"  that will get the xvclk CCF clock directly.
    "mclk"  that make ISI driver call his clock_start()/stop() to 
enable/disable ISI's peripheral clock.
If I only get xvclk clock, then the camera capture will be failed with a 
ISI timeout error.

But I think this is acceptable as we will keep go forward. Finally we'll 
switch to CCF and removed the v4l2_clock then we will move the 
clock_start()/stop() caller code to soc_camera.c.

Best Regards,
Josh Wu

>
> Thanks
> Guennadi
>
>> On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote:
>>> All uses of the v4l2_clk API so far only register one clock with a fixed
>>> name. This allows us to get rid of it, which also will make CCF and DT
>>> integration easier.
>>>
>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>> ---
>>>    drivers/media/platform/soc_camera/soc_camera.c |  6 +++---
>>>    drivers/media/usb/em28xx/em28xx-camera.c       |  2 +-
>>>    drivers/media/v4l2-core/v4l2-clk.c             | 24
>>> +++++++++++-------------
>>>    include/media/v4l2-clk.h                       |  7 +++----
>>>    4 files changed, 18 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c
>>> b/drivers/media/platform/soc_camera/soc_camera.c
>>> index f4be2a1..ce192b6 100644
>>> --- a/drivers/media/platform/soc_camera/soc_camera.c
>>> +++ b/drivers/media/platform/soc_camera/soc_camera.c
>>> @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct
>>> soc_camera_device *icd,
>>>    	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
>>>    		 shd->i2c_adapter_id, shd->board_info->addr);
>>>    -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk",
>>> icd);
>>> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>>>    	if (IS_ERR(icd->clk)) {
>>>    		ret = PTR_ERR(icd->clk);
>>>    		goto eclkreg;
>>> @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host
>>> *ici,
>>>    	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
>>>    		 sasd->asd.match.i2c.adapter_id, sasd->asd.match.i2c.address);
>>>    -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk",
>>> icd);
>>> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>>>    	if (IS_ERR(icd->clk)) {
>>>    		ret = PTR_ERR(icd->clk);
>>>    		goto eclkreg;
>>> @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
>>>    		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);
>>> +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
>>>    	if (IS_ERR(icd->clk)) {
>>>    		ret = PTR_ERR(icd->clk);
>>>    		goto eclkreg;
>>> diff --git a/drivers/media/usb/em28xx/em28xx-camera.c
>>> b/drivers/media/usb/em28xx/em28xx-camera.c
>>> index 7be661f..a4b22c2 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-camera.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-camera.c
>>> @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev)
>>>      	v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
>>>    			  i2c_adapter_id(adap), client->addr);
>>> -	v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL);
>>> +	v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL);
>>>    	if (IS_ERR(v4l2->clk))
>>>    		return PTR_ERR(v4l2->clk);
>>>    diff --git a/drivers/media/v4l2-core/v4l2-clk.c
>>> b/drivers/media/v4l2-core/v4l2-clk.c
>>> index e18cc04..c210906 100644
>>> --- a/drivers/media/v4l2-core/v4l2-clk.c
>>> +++ b/drivers/media/v4l2-core/v4l2-clk.c
>>> @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_id,
>>> const char *id)
>>>    		if (strcmp(dev_id, clk->dev_id))
>>>    			continue;
>>>    -		if (!id || !clk->id || !strcmp(clk->id, id))
>>> +		if ((!id && !clk->id) ||
>>> +		    (id && clk->id && !strcmp(clk->id, id)))
>>>    			return clk;
>>>    	}
>>>    @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
>>>    	mutex_lock(&clk->lock);
>>>      	enable = --clk->enable;
>>> -	if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__,
>>> -		 clk->dev_id, clk->id))
>>> +	if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__,
>>> +		 clk->dev_id))
>>>    		clk->enable++;
>>>    	else if (!enable && clk->ops->disable)
>>>    		clk->ops->disable(clk);
>>> @@ -181,7 +182,7 @@ EXPORT_SYMBOL(v4l2_clk_set_rate);
>>>      struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>>>    				   const char *dev_id,
>>> -				   const char *id, void *priv)
>>> +				   void *priv)
>>>    {
>>>    	struct v4l2_clk *clk;
>>>    	int ret;
>>> @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct
>>> v4l2_clk_ops *ops,
>>>    	if (!clk)
>>>    		return ERR_PTR(-ENOMEM);
>>>    -	clk->id = kstrdup(id, GFP_KERNEL);
>>>    	clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
>>> -	if ((id && !clk->id) || !clk->dev_id) {
>>> +	if (!clk->dev_id) {
>>>    		ret = -ENOMEM;
>>>    		goto ealloc;
>>>    	}
>>> @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct
>>> v4l2_clk_ops *ops,
>>>    	mutex_init(&clk->lock);
>>>      	mutex_lock(&clk_lock);
>>> -	if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
>>> +	if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) {
>> like mentioned in the beginning of the mail, you need to do same change in
>> v4l2_clk_get().
>>
>> Best Regards,
>> Josh Wu
>>
>>
>>>    		mutex_unlock(&clk_lock);
>>>    		ret = -EEXIST;
>>>    		goto eexist;
>>> @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct
>>> v4l2_clk_ops *ops,
>>>      eexist:
>>>    ealloc:
>>> -	kfree(clk->id);
>>>    	kfree(clk->dev_id);
>>>    	kfree(clk);
>>>    	return ERR_PTR(ret);
>>> @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register);
>>>    void v4l2_clk_unregister(struct v4l2_clk *clk)
>>>    {
>>>    	if (WARN(atomic_read(&clk->use_count),
>>> -		 "%s(): Refusing to unregister ref-counted %s:%s clock!\n",
>>> -		 __func__, clk->dev_id, clk->id))
>>> +		 "%s(): Refusing to unregister ref-counted %s clock!\n",
>>> +		 __func__, clk->dev_id))
>>>    		return;
>>>      	mutex_lock(&clk_lock);
>>>    	list_del(&clk->list);
>>>    	mutex_unlock(&clk_lock);
>>>    -	kfree(clk->id);
>>>    	kfree(clk->dev_id);
>>>    	kfree(clk);
>>>    }
>>> @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk
>>> *clk)
>>>    }
>>>      struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
>>> -		const char *id, unsigned long rate, struct module *owner)
>>> +				unsigned long rate, struct module *owner)
>>>    {
>>>    	struct v4l2_clk *clk;
>>>    	struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>>> @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const char
>>> *dev_id,
>>>    	priv->ops.get_rate = fixed_get_rate;
>>>    	priv->ops.owner = owner;
>>>    -	clk = v4l2_clk_register(&priv->ops, dev_id, id, priv);
>>> +	clk = v4l2_clk_register(&priv->ops, dev_id, priv);
>>>    	if (IS_ERR(clk))
>>>    		kfree(priv);
>>>    diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
>>> index 0b36cc1..8f06967 100644
>>> --- a/include/media/v4l2-clk.h
>>> +++ b/include/media/v4l2-clk.h
>>> @@ -43,7 +43,7 @@ struct v4l2_clk_ops {
>>>      struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
>>>    				   const char *dev_name,
>>> -				   const char *name, void *priv);
>>> +				   void *priv);
>>>    void v4l2_clk_unregister(struct v4l2_clk *clk);
>>>    struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id);
>>>    void v4l2_clk_put(struct v4l2_clk *clk);
>>> @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned
>>> long rate);
>>>    struct module;
>>>      struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
>>> -		const char *id, unsigned long rate, struct module *owner);
>>> +			unsigned long rate, struct module *owner);
>>>    void v4l2_clk_unregister_fixed(struct v4l2_clk *clk);
>>>      static inline struct v4l2_clk *v4l2_clk_register_fixed(const char
>>> *dev_id,
>>> -							const char *id,
>>>    							unsigned long rate)
>>>    {
>>> -	return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE);
>>> +	return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE);
>>>    }
>>>      #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name, size,
>>> \

Re: [PATCH 1/2] V4L: remove clock name from v4l2_clk API

[Guennadi Liakhovetski]

Hi Josh,

On Wed, 7 Jan 2015, Josh Wu wrote:

> Hi, Guennadi
> 
> On 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote:
> > Hi Josh,
> > 
> > On Tue, 6 Jan 2015, Josh Wu wrote:
> > 
> > > Hi, Guennadi
> > > 
> > > After look deep into this patch, I found you miss one line that should be
> > > changed as well.
> > > It's In function v4l2_clk_get(), there still has one line code called
> > > v4l2_clk_find(dev_id, id).
> > > You need to change it to v4l2_clk_find(dev_id, NULL) as well.
> > > Otherwise the code that many sensor used: v4l2_clk_get(&client->dev,
> > > "mclk")
> > > cannot acquired the "mclk" clock.
> > > 
> > > After above changes, this patch works for me.
> > I think you're right, in fact, since we now don't store CCF-based v4l2_clk
> > wrappers on the list, this can be simplified even further, I'll update the
> > patch. Did you only test this patch or both?
> I tested both patches with Atmel-isi driver. For the 2/2 patch I applied the
> modification Laurent suggested.
> Those patches works for me.
> 
> The only concern is in ov2640 I still need to acquired two v4l2 clocks:
>    "xvclk"  that will get the xvclk CCF clock directly.
>    "mclk"  that make ISI driver call his clock_start()/stop() to
> enable/disable ISI's peripheral clock.
> If I only get xvclk clock, then the camera capture will be failed with a ISI
> timeout error.

No, this doesn't look right to me. The camera sensor has only one clock 
input, so, it should only request one clock. Where does the clock signal 
to the camera come from on your system?

If it comes from the ISI itself, you don't need to specify the clock in 
the DT, since the ISI doesn't produce a clock from DT. If you do want to 
have your clock consumer (ov2640) and the supplier (ISI) properly 
described in DT, you'll have to teach the ISI to register a CCF clock 
source, which then will be connected to from the ov2640. If you choose not 
to show your clock in the DT, you can just use v4l2_clk_get(dev, "xvclk") 
and it will be handled by v4l2_clk / soc-camera / isi-atmel.

If the closk to ov2640 is supplied by a separate clock source, then you 
v4l2_clk_get() will connect ov2640 to it directly and soc-camera will 
enable and disable it on power-on / -off as required.

>From your above description it looks like the clock to ov2640 is supplied 
by a separate source, but atmel-isi's .clock_start() / .clock_stop() 
functions still need to be called? By looking at those functions it looks 
like they turn on and off clocks, supplying the ISI itself... Instead of 
only turning on and off clocks, provided by the ISI to a camera sensor. If 
my understanding is right, then this is a bug in atmel-isi and it has to 
be fixed.

Thanks
Guennadi

> But I think this is acceptable as we will keep go forward. Finally we'll
> switch to CCF and removed the v4l2_clock then we will move the
> clock_start()/stop() caller code to soc_camera.c.
> 
> Best Regards,
> Josh Wu
> 
> > 
> > Thanks
> > Guennadi
> > 
> > > On 1/2/2015 7:48 PM, Guennadi Liakhovetski wrote:
> > > > All uses of the v4l2_clk API so far only register one clock with a fixed
> > > > name. This allows us to get rid of it, which also will make CCF and DT
> > > > integration easier.
> > > > 
> > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > > ---
> > > >    drivers/media/platform/soc_camera/soc_camera.c |  6 +++---
> > > >    drivers/media/usb/em28xx/em28xx-camera.c       |  2 +-
> > > >    drivers/media/v4l2-core/v4l2-clk.c             | 24
> > > > +++++++++++-------------
> > > >    include/media/v4l2-clk.h                       |  7 +++----
> > > >    4 files changed, 18 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/platform/soc_camera/soc_camera.c
> > > > b/drivers/media/platform/soc_camera/soc_camera.c
> > > > index f4be2a1..ce192b6 100644
> > > > --- a/drivers/media/platform/soc_camera/soc_camera.c
> > > > +++ b/drivers/media/platform/soc_camera/soc_camera.c
> > > > @@ -1380,7 +1380,7 @@ static int soc_camera_i2c_init(struct
> > > > soc_camera_device *icd,
> > > >    	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
> > > >    		 shd->i2c_adapter_id, shd->board_info->addr);
> > > >    -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name,
> > > > "mclk",
> > > > icd);
> > > > +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
> > > >    	if (IS_ERR(icd->clk)) {
> > > >    		ret = PTR_ERR(icd->clk);
> > > >    		goto eclkreg;
> > > > @@ -1561,7 +1561,7 @@ static int scan_async_group(struct soc_camera_host
> > > > *ici,
> > > >    	snprintf(clk_name, sizeof(clk_name), "%d-%04x",
> > > >    		 sasd->asd.match.i2c.adapter_id,
> > > > sasd->asd.match.i2c.address);
> > > >    -	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name,
> > > > "mclk",
> > > > icd);
> > > > +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
> > > >    	if (IS_ERR(icd->clk)) {
> > > >    		ret = PTR_ERR(icd->clk);
> > > >    		goto eclkreg;
> > > > @@ -1666,7 +1666,7 @@ static int soc_of_bind(struct soc_camera_host
> > > > *ici,
> > > >    		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);
> > > > +	icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
> > > >    	if (IS_ERR(icd->clk)) {
> > > >    		ret = PTR_ERR(icd->clk);
> > > >    		goto eclkreg;
> > > > diff --git a/drivers/media/usb/em28xx/em28xx-camera.c
> > > > b/drivers/media/usb/em28xx/em28xx-camera.c
> > > > index 7be661f..a4b22c2 100644
> > > > --- a/drivers/media/usb/em28xx/em28xx-camera.c
> > > > +++ b/drivers/media/usb/em28xx/em28xx-camera.c
> > > > @@ -330,7 +330,7 @@ int em28xx_init_camera(struct em28xx *dev)
> > > >      	v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
> > > >    			  i2c_adapter_id(adap), client->addr);
> > > > -	v4l2->clk = v4l2_clk_register_fixed(clk_name, "mclk", -EINVAL);
> > > > +	v4l2->clk = v4l2_clk_register_fixed(clk_name, -EINVAL);
> > > >    	if (IS_ERR(v4l2->clk))
> > > >    		return PTR_ERR(v4l2->clk);
> > > >    diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> > > > b/drivers/media/v4l2-core/v4l2-clk.c
> > > > index e18cc04..c210906 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-clk.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-clk.c
> > > > @@ -31,7 +31,8 @@ static struct v4l2_clk *v4l2_clk_find(const char
> > > > *dev_id,
> > > > const char *id)
> > > >    		if (strcmp(dev_id, clk->dev_id))
> > > >    			continue;
> > > >    -		if (!id || !clk->id || !strcmp(clk->id, id))
> > > > +		if ((!id && !clk->id) ||
> > > > +		    (id && clk->id && !strcmp(clk->id, id)))
> > > >    			return clk;
> > > >    	}
> > > >    @@ -127,8 +128,8 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
> > > >    	mutex_lock(&clk->lock);
> > > >      	enable = --clk->enable;
> > > > -	if (WARN(enable < 0, "Unbalanced %s() on %s:%s!\n", __func__,
> > > > -		 clk->dev_id, clk->id))
> > > > +	if (WARN(enable < 0, "Unbalanced %s() on %s!\n", __func__,
> > > > +		 clk->dev_id))
> > > >    		clk->enable++;
> > > >    	else if (!enable && clk->ops->disable)
> > > >    		clk->ops->disable(clk);
> > > > @@ -181,7 +182,7 @@ EXPORT_SYMBOL(v4l2_clk_set_rate);
> > > >      struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
> > > >    				   const char *dev_id,
> > > > -				   const char *id, void *priv)
> > > > +				   void *priv)
> > > >    {
> > > >    	struct v4l2_clk *clk;
> > > >    	int ret;
> > > > @@ -193,9 +194,8 @@ struct v4l2_clk *v4l2_clk_register(const struct
> > > > v4l2_clk_ops *ops,
> > > >    	if (!clk)
> > > >    		return ERR_PTR(-ENOMEM);
> > > >    -	clk->id = kstrdup(id, GFP_KERNEL);
> > > >    	clk->dev_id = kstrdup(dev_id, GFP_KERNEL);
> > > > -	if ((id && !clk->id) || !clk->dev_id) {
> > > > +	if (!clk->dev_id) {
> > > >    		ret = -ENOMEM;
> > > >    		goto ealloc;
> > > >    	}
> > > > @@ -205,7 +205,7 @@ struct v4l2_clk *v4l2_clk_register(const struct
> > > > v4l2_clk_ops *ops,
> > > >    	mutex_init(&clk->lock);
> > > >      	mutex_lock(&clk_lock);
> > > > -	if (!IS_ERR(v4l2_clk_find(dev_id, id))) {
> > > > +	if (!IS_ERR(v4l2_clk_find(dev_id, NULL))) {
> > > like mentioned in the beginning of the mail, you need to do same change in
> > > v4l2_clk_get().
> > > 
> > > Best Regards,
> > > Josh Wu
> > > 
> > > 
> > > >    		mutex_unlock(&clk_lock);
> > > >    		ret = -EEXIST;
> > > >    		goto eexist;
> > > > @@ -217,7 +217,6 @@ struct v4l2_clk *v4l2_clk_register(const struct
> > > > v4l2_clk_ops *ops,
> > > >      eexist:
> > > >    ealloc:
> > > > -	kfree(clk->id);
> > > >    	kfree(clk->dev_id);
> > > >    	kfree(clk);
> > > >    	return ERR_PTR(ret);
> > > > @@ -227,15 +226,14 @@ EXPORT_SYMBOL(v4l2_clk_register);
> > > >    void v4l2_clk_unregister(struct v4l2_clk *clk)
> > > >    {
> > > >    	if (WARN(atomic_read(&clk->use_count),
> > > > -		 "%s(): Refusing to unregister ref-counted %s:%s clock!\n",
> > > > -		 __func__, clk->dev_id, clk->id))
> > > > +		 "%s(): Refusing to unregister ref-counted %s clock!\n",
> > > > +		 __func__, clk->dev_id))
> > > >    		return;
> > > >      	mutex_lock(&clk_lock);
> > > >    	list_del(&clk->list);
> > > >    	mutex_unlock(&clk_lock);
> > > >    -	kfree(clk->id);
> > > >    	kfree(clk->dev_id);
> > > >    	kfree(clk);
> > > >    }
> > > > @@ -253,7 +251,7 @@ static unsigned long fixed_get_rate(struct v4l2_clk
> > > > *clk)
> > > >    }
> > > >      struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
> > > > -		const char *id, unsigned long rate, struct module *owner)
> > > > +				unsigned long rate, struct module *owner)
> > > >    {
> > > >    	struct v4l2_clk *clk;
> > > >    	struct v4l2_clk_fixed *priv = kzalloc(sizeof(*priv),
> > > > GFP_KERNEL);
> > > > @@ -265,7 +263,7 @@ struct v4l2_clk *__v4l2_clk_register_fixed(const
> > > > char
> > > > *dev_id,
> > > >    	priv->ops.get_rate = fixed_get_rate;
> > > >    	priv->ops.owner = owner;
> > > >    -	clk = v4l2_clk_register(&priv->ops, dev_id, id, priv);
> > > > +	clk = v4l2_clk_register(&priv->ops, dev_id, priv);
> > > >    	if (IS_ERR(clk))
> > > >    		kfree(priv);
> > > >    diff --git a/include/media/v4l2-clk.h b/include/media/v4l2-clk.h
> > > > index 0b36cc1..8f06967 100644
> > > > --- a/include/media/v4l2-clk.h
> > > > +++ b/include/media/v4l2-clk.h
> > > > @@ -43,7 +43,7 @@ struct v4l2_clk_ops {
> > > >      struct v4l2_clk *v4l2_clk_register(const struct v4l2_clk_ops *ops,
> > > >    				   const char *dev_name,
> > > > -				   const char *name, void *priv);
> > > > +				   void *priv);
> > > >    void v4l2_clk_unregister(struct v4l2_clk *clk);
> > > >    struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id);
> > > >    void v4l2_clk_put(struct v4l2_clk *clk);
> > > > @@ -55,14 +55,13 @@ int v4l2_clk_set_rate(struct v4l2_clk *clk, unsigned
> > > > long rate);
> > > >    struct module;
> > > >      struct v4l2_clk *__v4l2_clk_register_fixed(const char *dev_id,
> > > > -		const char *id, unsigned long rate, struct module *owner);
> > > > +			unsigned long rate, struct module *owner);
> > > >    void v4l2_clk_unregister_fixed(struct v4l2_clk *clk);
> > > >      static inline struct v4l2_clk *v4l2_clk_register_fixed(const char
> > > > *dev_id,
> > > > -							const char *id,
> > > >    							unsigned long
> > > > rate)
> > > >    {
> > > > -	return __v4l2_clk_register_fixed(dev_id, id, rate, THIS_MODULE);
> > > > +	return __v4l2_clk_register_fixed(dev_id, rate, THIS_MODULE);
> > > >    }
> > > >      #define v4l2_clk_name_i2c(name, size, adap, client) snprintf(name,
> > > > size,
> > > > \
> 

Re: [PATCH 1/2] V4L: remove clock name from v4l2_clk API

[Laurent Pinchart]

Hi Guennadi and Josh,

On Thursday 08 January 2015 23:37:58 Guennadi Liakhovetski wrote:
> On Wed, 7 Jan 2015, Josh Wu wrote:
> > On 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote:
> >> On Tue, 6 Jan 2015, Josh Wu wrote:
> >>> Hi, Guennadi
> >>> 
> >>> After look deep into this patch, I found you miss one line that should
> >>> be changed as well.
> >>> It's In function v4l2_clk_get(), there still has one line code called
> >>> v4l2_clk_find(dev_id, id).
> >>> You need to change it to v4l2_clk_find(dev_id, NULL) as well.
> >>> Otherwise the code that many sensor used: v4l2_clk_get(&client->dev,
> >>> "mclk") cannot acquired the "mclk" clock.
> >>> 
> >>> After above changes, this patch works for me.
> >> 
> >> I think you're right, in fact, since we now don't store CCF-based
> >> v4l2_clk wrappers on the list, this can be simplified even further, I'll
> >> update the patch. Did you only test this patch or both?
> > 
> > I tested both patches with Atmel-isi driver. For the 2/2 patch I applied
> > the modification Laurent suggested.
> > Those patches works for me.
> > 
> > The only concern is in ov2640 I still need to acquired two v4l2 clocks:
> >    "xvclk"  that will get the xvclk CCF clock directly.
> >    "mclk"  that make ISI driver call his clock_start()/stop() to
> >    enable/disable ISI's peripheral clock.
> > If I only get xvclk clock, then the camera capture will be failed with a
> > ISI timeout error.
> 
> No, this doesn't look right to me. The camera sensor has only one clock
> input, so, it should only request one clock. Where does the clock signal
> to the camera come from on your system?

That's correct, the sensor driver only has one clock input, so it should just 
request the xvclk clock.

> If it comes from the ISI itself, you don't need to specify the clock in
> the DT, since the ISI doesn't produce a clock from DT. If you do want to
> have your clock consumer (ov2640) and the supplier (ISI) properly
> described in DT, you'll have to teach the ISI to register a CCF clock
> source, which then will be connected to from the ov2640. If you choose not
> to show your clock in the DT, you can just use v4l2_clk_get(dev, "xvclk")
> and it will be handled by v4l2_clk / soc-camera / isi-atmel.
>
> If the closk to ov2640 is supplied by a separate clock source, then you
> v4l2_clk_get() will connect ov2640 to it directly and soc-camera will
> enable and disable it on power-on / -off as required.

The ISI has no way to supply a sensor clock, the clock is supplied by a 
separate clock source.

> From your above description it looks like the clock to ov2640 is supplied
> by a separate source, but atmel-isi's .clock_start() / .clock_stop()
> functions still need to be called? By looking at those functions it looks
> like they turn on and off clocks, supplying the ISI itself... Instead of
> only turning on and off clocks, provided by the ISI to a camera sensor. If
> my understanding is right, then this is a bug in atmel-isi and it has to
> be fixed.

That's correct as well, the ISI driver needs to be fixed.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] V4L: remove clock name from v4l2_clk API

[Josh Wu]

Hi, Guennadi and Laurent

On 1/9/2015 6:47 AM, Laurent Pinchart wrote:
> Hi Guennadi and Josh,
>
> On Thursday 08 January 2015 23:37:58 Guennadi Liakhovetski wrote:
>> On Wed, 7 Jan 2015, Josh Wu wrote:
>>> On 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote:
>>>> On Tue, 6 Jan 2015, Josh Wu wrote:
>>>>> Hi, Guennadi
>>>>>
>>>>> After look deep into this patch, I found you miss one line that should
>>>>> be changed as well.
>>>>> It's In function v4l2_clk_get(), there still has one line code called
>>>>> v4l2_clk_find(dev_id, id).
>>>>> You need to change it to v4l2_clk_find(dev_id, NULL) as well.
>>>>> Otherwise the code that many sensor used: v4l2_clk_get(&client->dev,
>>>>> "mclk") cannot acquired the "mclk" clock.
>>>>>
>>>>> After above changes, this patch works for me.
>>>> I think you're right, in fact, since we now don't store CCF-based
>>>> v4l2_clk wrappers on the list, this can be simplified even further, I'll
>>>> update the patch. Did you only test this patch or both?
>>> I tested both patches with Atmel-isi driver. For the 2/2 patch I applied
>>> the modification Laurent suggested.
>>> Those patches works for me.
>>>
>>> The only concern is in ov2640 I still need to acquired two v4l2 clocks:
>>>     "xvclk"  that will get the xvclk CCF clock directly.
>>>     "mclk"  that make ISI driver call his clock_start()/stop() to
>>>     enable/disable ISI's peripheral clock.
>>> If I only get xvclk clock, then the camera capture will be failed with a
>>> ISI timeout error.
>> No, this doesn't look right to me. The camera sensor has only one clock
>> input, so, it should only request one clock. Where does the clock signal
>> to the camera come from on your system?
> That's correct, the sensor driver only has one clock input, so it should just
> request the xvclk clock.
>
>> If it comes from the ISI itself, you don't need to specify the clock in
>> the DT, since the ISI doesn't produce a clock from DT. If you do want to
>> have your clock consumer (ov2640) and the supplier (ISI) properly
>> described in DT, you'll have to teach the ISI to register a CCF clock
>> source, which then will be connected to from the ov2640. If you choose not
>> to show your clock in the DT, you can just use v4l2_clk_get(dev, "xvclk")
>> and it will be handled by v4l2_clk / soc-camera / isi-atmel.
>>
>> If the closk to ov2640 is supplied by a separate clock source, then you
>> v4l2_clk_get() will connect ov2640 to it directly and soc-camera will
>> enable and disable it on power-on / -off as required.
> The ISI has no way to supply a sensor clock, the clock is supplied by a
> separate clock source.
>
>>  From your above description it looks like the clock to ov2640 is supplied
>> by a separate source, but atmel-isi's .clock_start() / .clock_stop()
>> functions still need to be called? By looking at those functions it looks
>> like they turn on and off clocks, supplying the ISI itself... Instead of
>> only turning on and off clocks, provided by the ISI to a camera sensor. If
>> my understanding is right, then this is a bug in atmel-isi and it has to
>> be fixed.
> That's correct as well, the ISI driver needs to be fixed.
>

Thanks both of you for the details. Now I got it.
Indeed, I need fix this in atmel-isi driver not in ov2640 driver.
So I will send a new patch for this, which should move the ISI 
peripheral clock enable/disable() from clock_start/stop() to 
isi_camera_add_device/remove_device().

Best Regards,
Josh Wu

Re: [PATCH 1/2] V4L: remove clock name from v4l2_clk API

[Laurent Pinchart]

Hi Josh,

On Monday 12 January 2015 17:14:33 Josh Wu wrote:
> On 1/9/2015 6:47 AM, Laurent Pinchart wrote:
> > On Thursday 08 January 2015 23:37:58 Guennadi Liakhovetski wrote:
> >> On Wed, 7 Jan 2015, Josh Wu wrote:
> >>> On 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote:
> >>>> On Tue, 6 Jan 2015, Josh Wu wrote:
> >>>>> Hi, Guennadi
> >>>>> 
> >>>>> After look deep into this patch, I found you miss one line that should
> >>>>> be changed as well.
> >>>>> It's In function v4l2_clk_get(), there still has one line code called
> >>>>> v4l2_clk_find(dev_id, id).
> >>>>> You need to change it to v4l2_clk_find(dev_id, NULL) as well.
> >>>>> Otherwise the code that many sensor used: v4l2_clk_get(&client->dev,
> >>>>> "mclk") cannot acquired the "mclk" clock.
> >>>>> 
> >>>>> After above changes, this patch works for me.
> >>>> 
> >>>> I think you're right, in fact, since we now don't store CCF-based
> >>>> v4l2_clk wrappers on the list, this can be simplified even further,
> >>>> I'll update the patch. Did you only test this patch or both?
> >>> 
> >>> I tested both patches with Atmel-isi driver. For the 2/2 patch I applied
> >>> the modification Laurent suggested.
> >>> Those patches works for me.
> >>> 
> >>> The only concern is in ov2640 I still need to acquired two v4l2 clocks:
> >>>     "xvclk"  that will get the xvclk CCF clock directly.
> >>>     "mclk"  that make ISI driver call his clock_start()/stop() to
> >>>     enable/disable ISI's peripheral clock.
> >>> 
> >>> If I only get xvclk clock, then the camera capture will be failed with a
> >>> ISI timeout error.
> >> 
> >> No, this doesn't look right to me. The camera sensor has only one clock
> >> input, so, it should only request one clock. Where does the clock signal
> >> to the camera come from on your system?
> > 
> > That's correct, the sensor driver only has one clock input, so it should
> > just request the xvclk clock.
> > 
> >> If it comes from the ISI itself, you don't need to specify the clock in
> >> the DT, since the ISI doesn't produce a clock from DT. If you do want to
> >> have your clock consumer (ov2640) and the supplier (ISI) properly
> >> described in DT, you'll have to teach the ISI to register a CCF clock
> >> source, which then will be connected to from the ov2640. If you choose
> >> not to show your clock in the DT, you can just use v4l2_clk_get(dev,
> >> "xvclk") and it will be handled by v4l2_clk / soc-camera / isi-atmel.
> >> 
> >> If the closk to ov2640 is supplied by a separate clock source, then you
> >> v4l2_clk_get() will connect ov2640 to it directly and soc-camera will
> >> enable and disable it on power-on / -off as required.
> > 
> > The ISI has no way to supply a sensor clock, the clock is supplied by a
> > separate clock source.
> > 
> >> From your above description it looks like the clock to ov2640 is
> >> supplied by a separate source, but atmel-isi's .clock_start() /
> >> .clock_stop() functions still need to be called? By looking at those
> >> functions it looks like they turn on and off clocks, supplying the ISI
> >> itself... Instead of only turning on and off clocks, provided by the ISI
> >> to a camera sensor. If my understanding is right, then this is a bug in
> >> atmel-isi and it has to be fixed.
> > 
> > That's correct as well, the ISI driver needs to be fixed.
> 
> Thanks both of you for the details. Now I got it.
> Indeed, I need fix this in atmel-isi driver not in ov2640 driver.
> So I will send a new patch for this, which should move the ISI
> peripheral clock enable/disable() from clock_start/stop() to
> isi_camera_add_device/remove_device().

Shouldn't you move it to the start_streaming() and stop_streaming() functions 
instead ? An even better solution would be to use runtime PM to enable/disable 
the ISI clock in the runtime PM resume and suspend handlers, and call 
pm_runtime_get_sync() and pm_runtime_put() when you need the ISI to be 
operational.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] V4L: remove clock name from v4l2_clk API

[Josh Wu]

On 1/12/2015 6:38 PM, Laurent Pinchart wrote:
> Hi Josh,
>
> On Monday 12 January 2015 17:14:33 Josh Wu wrote:
>> On 1/9/2015 6:47 AM, Laurent Pinchart wrote:
>>> On Thursday 08 January 2015 23:37:58 Guennadi Liakhovetski wrote:
>>>> On Wed, 7 Jan 2015, Josh Wu wrote:
>>>>> On 1/7/2015 6:17 AM, Guennadi Liakhovetski wrote:
>>>>>> On Tue, 6 Jan 2015, Josh Wu wrote:
>>>>>>> Hi, Guennadi
>>>>>>>
>>>>>>> After look deep into this patch, I found you miss one line that should
>>>>>>> be changed as well.
>>>>>>> It's In function v4l2_clk_get(), there still has one line code called
>>>>>>> v4l2_clk_find(dev_id, id).
>>>>>>> You need to change it to v4l2_clk_find(dev_id, NULL) as well.
>>>>>>> Otherwise the code that many sensor used: v4l2_clk_get(&client->dev,
>>>>>>> "mclk") cannot acquired the "mclk" clock.
>>>>>>>
>>>>>>> After above changes, this patch works for me.
>>>>>> I think you're right, in fact, since we now don't store CCF-based
>>>>>> v4l2_clk wrappers on the list, this can be simplified even further,
>>>>>> I'll update the patch. Did you only test this patch or both?
>>>>> I tested both patches with Atmel-isi driver. For the 2/2 patch I applied
>>>>> the modification Laurent suggested.
>>>>> Those patches works for me.
>>>>>
>>>>> The only concern is in ov2640 I still need to acquired two v4l2 clocks:
>>>>>      "xvclk"  that will get the xvclk CCF clock directly.
>>>>>      "mclk"  that make ISI driver call his clock_start()/stop() to
>>>>>      enable/disable ISI's peripheral clock.
>>>>>
>>>>> If I only get xvclk clock, then the camera capture will be failed with a
>>>>> ISI timeout error.
>>>> No, this doesn't look right to me. The camera sensor has only one clock
>>>> input, so, it should only request one clock. Where does the clock signal
>>>> to the camera come from on your system?
>>> That's correct, the sensor driver only has one clock input, so it should
>>> just request the xvclk clock.
>>>
>>>> If it comes from the ISI itself, you don't need to specify the clock in
>>>> the DT, since the ISI doesn't produce a clock from DT. If you do want to
>>>> have your clock consumer (ov2640) and the supplier (ISI) properly
>>>> described in DT, you'll have to teach the ISI to register a CCF clock
>>>> source, which then will be connected to from the ov2640. If you choose
>>>> not to show your clock in the DT, you can just use v4l2_clk_get(dev,
>>>> "xvclk") and it will be handled by v4l2_clk / soc-camera / isi-atmel.
>>>>
>>>> If the closk to ov2640 is supplied by a separate clock source, then you
>>>> v4l2_clk_get() will connect ov2640 to it directly and soc-camera will
>>>> enable and disable it on power-on / -off as required.
>>> The ISI has no way to supply a sensor clock, the clock is supplied by a
>>> separate clock source.
>>>
>>>>  From your above description it looks like the clock to ov2640 is
>>>> supplied by a separate source, but atmel-isi's .clock_start() /
>>>> .clock_stop() functions still need to be called? By looking at those
>>>> functions it looks like they turn on and off clocks, supplying the ISI
>>>> itself... Instead of only turning on and off clocks, provided by the ISI
>>>> to a camera sensor. If my understanding is right, then this is a bug in
>>>> atmel-isi and it has to be fixed.
>>> That's correct as well, the ISI driver needs to be fixed.
>> Thanks both of you for the details. Now I got it.
>> Indeed, I need fix this in atmel-isi driver not in ov2640 driver.
>> So I will send a new patch for this, which should move the ISI
>> peripheral clock enable/disable() from clock_start/stop() to
>> isi_camera_add_device/remove_device().
> Shouldn't you move it to the start_streaming() and stop_streaming() functions
> instead ? An even better solution would be to use runtime PM to enable/disable
> the ISI clock in the runtime PM resume and suspend handlers, and call
> pm_runtime_get_sync() and pm_runtime_put() when you need the ISI to be
> operational.

Okay, I'll try to add the PM functions for atmel-isi in the meantime.

Best Regards,
Josh Wu
>