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

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

[Guennadi Liakhovetski]

Hi,

A v3 of a CCF support for v4l2_clk. Comments addressed.

Thanks
Guennadi

[PATCH v3 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>
---

v3: .id field removed from the struct. Since CCF clocks won't be added to 
the V4L2 clock list at all in patch 2 in this series, no clock ID 
comparison is needed in v4l2_clk_find() either.

 drivers/media/platform/soc_camera/soc_camera.c |  6 ++---
 drivers/media/usb/em28xx/em28xx-camera.c       |  2 +-
 drivers/media/v4l2-core/v4l2-clk.c             | 33 ++++++++++----------------
 include/media/v4l2-clk.h                       |  8 +++----
 4 files changed, 20 insertions(+), 29 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..3ff0b00 100644
--- a/drivers/media/v4l2-core/v4l2-clk.c
+++ b/drivers/media/v4l2-core/v4l2-clk.c
@@ -23,17 +23,13 @@
 static DEFINE_MUTEX(clk_lock);
 static LIST_HEAD(clk_list);
 
-static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id)
+static struct v4l2_clk *v4l2_clk_find(const char *dev_id)
 {
 	struct v4l2_clk *clk;
 
-	list_for_each_entry(clk, &clk_list, list) {
-		if (strcmp(dev_id, clk->dev_id))
-			continue;
-
-		if (!id || !clk->id || !strcmp(clk->id, id))
+	list_for_each_entry(clk, &clk_list, list)
+		if (!strcmp(dev_id, clk->dev_id))
 			return clk;
-	}
 
 	return ERR_PTR(-ENODEV);
 }
@@ -43,7 +39,7 @@ struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id)
 	struct v4l2_clk *clk;
 
 	mutex_lock(&clk_lock);
-	clk = v4l2_clk_find(dev_name(dev), id);
+	clk = v4l2_clk_find(dev_name(dev));
 
 	if (!IS_ERR(clk))
 		atomic_inc(&clk->use_count);
@@ -127,8 +123,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 +177,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 +189,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 +200,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))) {
 		mutex_unlock(&clk_lock);
 		ret = -EEXIST;
 		goto eexist;
@@ -217,7 +212,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 +221,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 +246,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 +258,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..928045f 100644
--- a/include/media/v4l2-clk.h
+++ b/include/media/v4l2-clk.h
@@ -26,7 +26,6 @@ struct v4l2_clk {
 	struct list_head list;
 	const struct v4l2_clk_ops *ops;
 	const char *dev_id;
-	const char *id;
 	int enable;
 	struct mutex lock; /* Protect the enable count */
 	atomic_t use_count;
@@ -43,7 +42,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 +54,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 v3 2/2] V4L: 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>
---

v3:
1. return -EPROBE_DEFER if it's returned by clk_get()
2. handle the case of disabled CCF in kernel configuration
3. use clk_prepare_enable() and clk_unprepare_disable()

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

diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
index 3ff0b00..9f8cb20 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>
@@ -37,6 +38,21 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_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 (PTR_ERR(ccf_clk) == -EPROBE_DEFER)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	if (!IS_ERR_OR_NULL(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));
@@ -56,6 +72,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)
@@ -93,8 +115,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_prepare_enable(clk->clk);
+
+	ret = v4l2_clk_lock_driver(clk);
 	if (ret < 0)
 		return ret;
 
@@ -120,6 +146,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
 {
 	int enable;
 
+	if (clk->clk)
+		return clk_disable_unprepare(clk->clk);
+
 	mutex_lock(&clk->lock);
 
 	enable = --clk->enable;
@@ -137,8 +166,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;
 
@@ -157,7 +190,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 928045f..3ef6e3d 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;
@@ -29,6 +30,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 v3 2/2] V4L: add CCF support to the v4l2_clk API

[Laurent Pinchart]

Hi Guennadi,

Thank you for the patch.

On Sunday 01 February 2015 00:21:36 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>
> ---
> 
> v3:
> 1. return -EPROBE_DEFER if it's returned by clk_get()
> 2. handle the case of disabled CCF in kernel configuration
> 3. use clk_prepare_enable() and clk_unprepare_disable()
> 
>  drivers/media/v4l2-core/v4l2-clk.c | 48 ++++++++++++++++++++++++++++++++---
>  include/media/v4l2-clk.h           |  2 ++
>  2 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> b/drivers/media/v4l2-core/v4l2-clk.c index 3ff0b00..9f8cb20 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>
> @@ -37,6 +38,21 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_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 (PTR_ERR(ccf_clk) == -EPROBE_DEFER)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	if (!IS_ERR_OR_NULL(ccf_clk)) {
> +		clk = kzalloc(sizeof(struct v4l2_clk), GFP_KERNEL);

Doesn't the kernel tend to favour sizeof(*clk) instead of sizeof(struct 
v4l2_clk) ?

Apart from that,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		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));
> @@ -56,6 +72,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)
> @@ -93,8 +115,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_prepare_enable(clk->clk);
> +
> +	ret = v4l2_clk_lock_driver(clk);
>  	if (ret < 0)
>  		return ret;
> 
> @@ -120,6 +146,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
>  {
>  	int enable;
> 
> +	if (clk->clk)
> +		return clk_disable_unprepare(clk->clk);
> +
>  	mutex_lock(&clk->lock);
> 
>  	enable = --clk->enable;
> @@ -137,8 +166,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;
> 
> @@ -157,7 +190,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 928045f..3ef6e3d 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;
> @@ -29,6 +30,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 v3 1/2] V4L: remove clock name from v4l2_clk API

[Laurent Pinchart]

Hi Guennadi,

Thank you for the patch.

On Sunday 01 February 2015 00:21:32 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>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> v3: .id field removed from the struct. Since CCF clocks won't be added to
> the V4L2 clock list at all in patch 2 in this series, no clock ID
> comparison is needed in v4l2_clk_find() either.
> 
>  drivers/media/platform/soc_camera/soc_camera.c |  6 ++---
>  drivers/media/usb/em28xx/em28xx-camera.c       |  2 +-
>  drivers/media/v4l2-core/v4l2-clk.c             | 33 +++++++++--------------
>  include/media/v4l2-clk.h                       |  8 +++----
>  4 files changed, 20 insertions(+), 29 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..3ff0b00 100644
> --- a/drivers/media/v4l2-core/v4l2-clk.c
> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> @@ -23,17 +23,13 @@
>  static DEFINE_MUTEX(clk_lock);
>  static LIST_HEAD(clk_list);
> 
> -static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id)
> +static struct v4l2_clk *v4l2_clk_find(const char *dev_id)
>  {
>  	struct v4l2_clk *clk;
> 
> -	list_for_each_entry(clk, &clk_list, list) {
> -		if (strcmp(dev_id, clk->dev_id))
> -			continue;
> -
> -		if (!id || !clk->id || !strcmp(clk->id, id))
> +	list_for_each_entry(clk, &clk_list, list)
> +		if (!strcmp(dev_id, clk->dev_id))
>  			return clk;
> -	}
> 
>  	return ERR_PTR(-ENODEV);
>  }
> @@ -43,7 +39,7 @@ struct v4l2_clk *v4l2_clk_get(struct device *dev, const
> char *id) struct v4l2_clk *clk;
> 
>  	mutex_lock(&clk_lock);
> -	clk = v4l2_clk_find(dev_name(dev), id);
> +	clk = v4l2_clk_find(dev_name(dev));
> 
>  	if (!IS_ERR(clk))
>  		atomic_inc(&clk->use_count);
> @@ -127,8 +123,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 +177,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 +189,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 +200,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))) {
>  		mutex_unlock(&clk_lock);
>  		ret = -EEXIST;
>  		goto eexist;
> @@ -217,7 +212,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 +221,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 +246,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 +258,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..928045f 100644
> --- a/include/media/v4l2-clk.h
> +++ b/include/media/v4l2-clk.h
> @@ -26,7 +26,6 @@ struct v4l2_clk {
>  	struct list_head list;
>  	const struct v4l2_clk_ops *ops;
>  	const char *dev_id;
> -	const char *id;
>  	int enable;
>  	struct mutex lock; /* Protect the enable count */
>  	atomic_t use_count;
> @@ -43,7 +42,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 +54,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, \

-- 
Regards,

Laurent Pinchart

[PATCH v4 2/2] V4L: 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>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

v4: sizeof(*clk) :)

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

diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
index 3ff0b00..9f8cb20 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>
@@ -37,6 +38,21 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_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 (PTR_ERR(ccf_clk) == -EPROBE_DEFER)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	if (!IS_ERR_OR_NULL(ccf_clk)) {
+		clk = kzalloc(sizeof(*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));
@@ -56,6 +72,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)
@@ -93,8 +115,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_prepare_enable(clk->clk);
+
+	ret = v4l2_clk_lock_driver(clk);
 	if (ret < 0)
 		return ret;
 
@@ -120,6 +146,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
 {
 	int enable;
 
+	if (clk->clk)
+		return clk_disable_unprepare(clk->clk);
+
 	mutex_lock(&clk->lock);
 
 	enable = --clk->enable;
@@ -137,8 +166,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;
 
@@ -157,7 +190,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 928045f..3ef6e3d 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;
@@ -29,6 +30,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 v3 1/2] V4L: remove clock name from v4l2_clk API

[Josh Wu]

Hi, Guennadi

On 2/1/2015 7:21 AM, 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>
> ---

Thanks for the patch.
Tested-by: Josh Wu <josh.wu@atmel.com>

Best Regards,
Josh Wu

>
> v3: .id field removed from the struct. Since CCF clocks won't be added to
> the V4L2 clock list at all in patch 2 in this series, no clock ID
> comparison is needed in v4l2_clk_find() either.
>
>   drivers/media/platform/soc_camera/soc_camera.c |  6 ++---
>   drivers/media/usb/em28xx/em28xx-camera.c       |  2 +-
>   drivers/media/v4l2-core/v4l2-clk.c             | 33 ++++++++++----------------
>   include/media/v4l2-clk.h                       |  8 +++----
>   4 files changed, 20 insertions(+), 29 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..3ff0b00 100644
> --- a/drivers/media/v4l2-core/v4l2-clk.c
> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> @@ -23,17 +23,13 @@
>   static DEFINE_MUTEX(clk_lock);
>   static LIST_HEAD(clk_list);
>   
> -static struct v4l2_clk *v4l2_clk_find(const char *dev_id, const char *id)
> +static struct v4l2_clk *v4l2_clk_find(const char *dev_id)
>   {
>   	struct v4l2_clk *clk;
>   
> -	list_for_each_entry(clk, &clk_list, list) {
> -		if (strcmp(dev_id, clk->dev_id))
> -			continue;
> -
> -		if (!id || !clk->id || !strcmp(clk->id, id))
> +	list_for_each_entry(clk, &clk_list, list)
> +		if (!strcmp(dev_id, clk->dev_id))
>   			return clk;
> -	}
>   
>   	return ERR_PTR(-ENODEV);
>   }
> @@ -43,7 +39,7 @@ struct v4l2_clk *v4l2_clk_get(struct device *dev, const char *id)
>   	struct v4l2_clk *clk;
>   
>   	mutex_lock(&clk_lock);
> -	clk = v4l2_clk_find(dev_name(dev), id);
> +	clk = v4l2_clk_find(dev_name(dev));
>   
>   	if (!IS_ERR(clk))
>   		atomic_inc(&clk->use_count);
> @@ -127,8 +123,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 +177,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 +189,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 +200,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))) {
>   		mutex_unlock(&clk_lock);
>   		ret = -EEXIST;
>   		goto eexist;
> @@ -217,7 +212,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 +221,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 +246,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 +258,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..928045f 100644
> --- a/include/media/v4l2-clk.h
> +++ b/include/media/v4l2-clk.h
> @@ -26,7 +26,6 @@ struct v4l2_clk {
>   	struct list_head list;
>   	const struct v4l2_clk_ops *ops;
>   	const char *dev_id;
> -	const char *id;
>   	int enable;
>   	struct mutex lock; /* Protect the enable count */
>   	atomic_t use_count;
> @@ -43,7 +42,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 +54,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 v4 2/2] V4L: add CCF support to the v4l2_clk API

[Josh Wu]

Hi, Guennadi

On 2/1/2015 7:12 PM, 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>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks for the patch.
Tested-by: Josh Wu <josh.wu@atmel.com>

Best Regards,
Josh Wu

> ---
>
> v4: sizeof(*clk) :)
>
>   drivers/media/v4l2-core/v4l2-clk.c | 48 +++++++++++++++++++++++++++++++++++---
>   include/media/v4l2-clk.h           |  2 ++
>   2 files changed, 47 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> index 3ff0b00..9f8cb20 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>
> @@ -37,6 +38,21 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_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 (PTR_ERR(ccf_clk) == -EPROBE_DEFER)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	if (!IS_ERR_OR_NULL(ccf_clk)) {
> +		clk = kzalloc(sizeof(*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));
> @@ -56,6 +72,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)
> @@ -93,8 +115,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_prepare_enable(clk->clk);
> +
> +	ret = v4l2_clk_lock_driver(clk);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -120,6 +146,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
>   {
>   	int enable;
>   
> +	if (clk->clk)
> +		return clk_disable_unprepare(clk->clk);
> +
>   	mutex_lock(&clk->lock);
>   
>   	enable = --clk->enable;
> @@ -137,8 +166,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;
>   
> @@ -157,7 +190,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 928045f..3ef6e3d 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;
> @@ -29,6 +30,7 @@ struct v4l2_clk {
>   	int enable;
>   	struct mutex lock; /* Protect the enable count */
>   	atomic_t use_count;
> +	struct clk *clk;
>   	void *priv;
>   };
>   

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

[Mauro Carvalho Chehab]

Em Sun, 1 Feb 2015 12:12:33 +0100 (CET)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> escreveu:

> 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>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> 
> v4: sizeof(*clk) :)
> 
>  drivers/media/v4l2-core/v4l2-clk.c | 48 +++++++++++++++++++++++++++++++++++---
>  include/media/v4l2-clk.h           |  2 ++
>  2 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> index 3ff0b00..9f8cb20 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>
> @@ -37,6 +38,21 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_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 (PTR_ERR(ccf_clk) == -EPROBE_DEFER)
> +		return ERR_PTR(-EPROBE_DEFER);

Why not do just:
		return ccf_clk;

> +
> +	if (!IS_ERR_OR_NULL(ccf_clk)) {
> +		clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> +		if (!clk) {
> +			clk_put(ccf_clk);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +		clk->clk = ccf_clk;
> +
> +		return clk;
> +	}

The error condition here looks a little weird to me. I mean, if the
CCF clock returns an error, shouldn't it fail instead of silently
run some logic to find another clock source? Isn't it risky on getting
a wrong value?

If the above code is right, please add a comment there explaining
why it is safe to discard the CCF clock error.

>  
>  	mutex_lock(&clk_lock);
>  	clk = v4l2_clk_find(dev_name(dev));
> @@ -56,6 +72,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)
> @@ -93,8 +115,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_prepare_enable(clk->clk);
> +
> +	ret = v4l2_clk_lock_driver(clk);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -120,6 +146,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
>  {
>  	int enable;
>  
> +	if (clk->clk)
> +		return clk_disable_unprepare(clk->clk);
> +
>  	mutex_lock(&clk->lock);
>  
>  	enable = --clk->enable;
> @@ -137,8 +166,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;
>  
> @@ -157,7 +190,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 928045f..3ef6e3d 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;
> @@ -29,6 +30,7 @@ struct v4l2_clk {
>  	int enable;
>  	struct mutex lock; /* Protect the enable count */
>  	atomic_t use_count;
> +	struct clk *clk;
>  	void *priv;
>  };
>  

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

[laurent.pinchart]

Hi Mauro,

On Mon Mar 02 2015 18:55:23 GMT+0200 (EET), Mauro Carvalho Chehab wrote:
> Em Sun, 1 Feb 2015 12:12:33 +0100 (CET)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> escreveu:
> 
> > 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>
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> > v4: sizeof(*clk) :)
> > 
> >  drivers/media/v4l2-core/v4l2-clk.c | 48 +++++++++++++++++++++++++++++++++++---
> >  include/media/v4l2-clk.h           |  2 ++
> >  2 files changed, 47 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> > index 3ff0b00..9f8cb20 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>
> > @@ -37,6 +38,21 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_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 (PTR_ERR(ccf_clk) == -EPROBE_DEFER)
> > +		return ERR_PTR(-EPROBE_DEFER);
> 
> Why not do just:
> 		return ccf_clk;

I find the explicit error slightly more readable, but that's a matter of taste.
 
> > +
> > +	if (!IS_ERR_OR_NULL(ccf_clk)) {
> > +		clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> > +		if (!clk) {
> > +			clk_put(ccf_clk);
> > +			return ERR_PTR(-ENOMEM);
> > +		}
> > +		clk->clk = ccf_clk;
> > +
> > +		return clk;
> > +	}
> 
> The error condition here looks a little weird to me. I mean, if the
> CCF clock returns an error, shouldn't it fail instead of silently
> run some logic to find another clock source? Isn't it risky on getting
> a wrong value?

The idea is that, in the long term, everything should use CCF directly. However, we have clock providers on platforms where CCF isn't avalaible. V4L2 clock has been introduced  as a  single API usable by V4L2 clock users allowing them to retrieve and use clocks regardless of whether the provider uses CCF or not. Internally it first tries CCF, and then falls back to the non-CCF implementation in case of failure. 
 
> If the above code is right, please add a comment there explaining
> why it is safe to discard the CCF clock error.
> 
> >  
> >  	mutex_lock(&clk_lock);
> >  	clk = v4l2_clk_find(dev_name(dev));
> > @@ -56,6 +72,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)
> > @@ -93,8 +115,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_prepare_enable(clk->clk);
> > +
> > +	ret = v4l2_clk_lock_driver(clk);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -120,6 +146,9 @@ void v4l2_clk_disable(struct v4l2_clk *clk)
> >  {
> >  	int enable;
> >  
> > +	if (clk->clk)
> > +		return clk_disable_unprepare(clk->clk);
> > +
> >  	mutex_lock(&clk->lock);
> >  
> >  	enable = --clk->enable;
> > @@ -137,8 +166,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;
> >  
> > @@ -157,7 +190,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 928045f..3ef6e3d 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;
> > @@ -29,6 +30,7 @@ struct v4l2_clk {
> >  	int enable;
> >  	struct mutex lock; /* Protect the enable count */
> >  	atomic_t use_count;
> > +	struct clk *clk;
> >  	void *priv;
> >  };
> >  
>

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

[Mauro Carvalho Chehab]

Em Mon, 02 Mar 2015 20:52:41 +0000
laurent.pinchart@ideasonboard.com escreveu:

> Hi Mauro,
> 
> On Mon Mar 02 2015 18:55:23 GMT+0200 (EET), Mauro Carvalho Chehab wrote:
> > Em Sun, 1 Feb 2015 12:12:33 +0100 (CET)
> > Guennadi Liakhovetski <g.liakhovetski@gmx.de> escreveu:
> > 
> > > 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>
> > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > > v4: sizeof(*clk) :)
> > > 
> > >  drivers/media/v4l2-core/v4l2-clk.c | 48 +++++++++++++++++++++++++++++++++++---
> > >  include/media/v4l2-clk.h           |  2 ++
> > >  2 files changed, 47 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> > > index 3ff0b00..9f8cb20 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>
> > > @@ -37,6 +38,21 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_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 (PTR_ERR(ccf_clk) == -EPROBE_DEFER)
> > > +		return ERR_PTR(-EPROBE_DEFER);
> > 
> > Why not do just:
> > 		return ccf_clk;
> 
> I find the explicit error slightly more readable, but that's a matter of taste.

Well, return(ccf_clk) will likely produce a smaller instruction code
than return (long).

>  
> > > +
> > > +	if (!IS_ERR_OR_NULL(ccf_clk)) {
> > > +		clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> > > +		if (!clk) {
> > > +			clk_put(ccf_clk);
> > > +			return ERR_PTR(-ENOMEM);
> > > +		}
> > > +		clk->clk = ccf_clk;
> > > +
> > > +		return clk;
> > > +	}
> > 
> > The error condition here looks a little weird to me. I mean, if the
> > CCF clock returns an error, shouldn't it fail instead of silently
> > run some logic to find another clock source? Isn't it risky on getting
> > a wrong value?
> 
> The idea is that, in the long term, everything should use CCF directly. However, we have clock providers on platforms where CCF isn't avalaible. V4L2 clock has been introduced  as a  single API usable by V4L2 clock users allowing them to retrieve and use clocks regardless of whether the provider uses CCF or not. Internally it first tries CCF, and then falls back to the non-CCF implementation in case of failure. 

Yeah, I got that the non-CCF is a fallback code, to be used on
platforms that CCF isn't available.

However, the above code doesn't seem to look if CCF is available
or not. Instead, it assumes that *all* error codes, or even NULL,
means that CCF isn't available.

Shouldn't it be, instead, either waiting for NULL or for some
specific error code, in order to:
- return the error code, if CCF is available but getting
  the clock failed;
- run the backward-compat code when CCF is not available.

Regards,
Mauro

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

[Laurent Pinchart]

Hi Mauro,

On Tuesday 03 March 2015 13:40:50 Mauro Carvalho Chehab wrote:
> Em Mon, 02 Mar 2015 20:52:41 +0000 Laurent Pinchart escreveu:
> > On Mon Mar 02 2015 18:55:23 GMT+0200 (EET), Mauro Carvalho Chehab wrote:
> >> Em Sun, 1 Feb 2015 12:12:33 +0100 (CET) Guennadi Liakhovetski escreveu:
> >>> 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>
> >>> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>> 
> >>> v4: sizeof(*clk) :)
> >>> 
> >>>  drivers/media/v4l2-core/v4l2-clk.c | 48 ++++++++++++++++++++++++++---
> >>>  include/media/v4l2-clk.h           |  2 ++
> >>>  2 files changed, 47 insertions(+), 3 deletions(-)
> >>> 
> >>> diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> >>> b/drivers/media/v4l2-core/v4l2-clk.c index 3ff0b00..9f8cb20 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-clk.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-clk.c

[snip]

> >>> @@ -37,6 +38,21 @@ static struct v4l2_clk *v4l2_clk_find(const char
> >>> *dev_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 (PTR_ERR(ccf_clk) == -EPROBE_DEFER)
> >>> +		return ERR_PTR(-EPROBE_DEFER);
> >> 
> >> Why not do just:
> >> 		return ccf_clk;
> > 
> > I find the explicit error slightly more readable, but that's a matter of
> > taste.
>
> Well, return(ccf_clk) will likely produce a smaller instruction code
> than return (long).

Not if the compiler is smart :-)

> >>> +
> >>> +	if (!IS_ERR_OR_NULL(ccf_clk)) {
> >>> +		clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> >>> +		if (!clk) {
> >>> +			clk_put(ccf_clk);
> >>> +			return ERR_PTR(-ENOMEM);
> >>> +		}
> >>> +		clk->clk = ccf_clk;
> >>> +
> >>> +		return clk;
> >>> +	}
> >> 
> >> The error condition here looks a little weird to me. I mean, if the
> >> CCF clock returns an error, shouldn't it fail instead of silently
> >> run some logic to find another clock source? Isn't it risky on getting
> >> a wrong value?
> > 
> > The idea is that, in the long term, everything should use CCF directly.
> > However, we have clock providers on platforms where CCF isn't avalaible.
> > V4L2 clock has been introduced  as a  single API usable by V4L2 clock
> > users allowing them to retrieve and use clocks regardless of whether the
> > provider uses CCF or not. Internally it first tries CCF, and then falls
> > back to the non-CCF implementation in case of failure.
>
> Yeah, I got that the non-CCF is a fallback code, to be used on
> platforms that CCF isn't available.
> 
> However, the above code doesn't seem to look if CCF is available
> or not. Instead, it assumes that *all* error codes, or even NULL,
> means that CCF isn't available.
> 
> Shouldn't it be, instead, either waiting for NULL or for some
> specific error code, in order to:
> - return the error code, if CCF is available but getting
>   the clock failed;
> - run the backward-compat code when CCF is not available.

Isn't that pretty much what the code is doing ? If we get a -EPROBE_DEFER 
error from CCF meaning that the clock is known but not registered yet we 
return it. Otherwise, if the clock is unknown to CCF, or if CCF is disabled, 
we fall back.

-- 
Regards,

Laurent Pinchart

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

[Mauro Carvalho Chehab]

Em Wed, 04 Mar 2015 00:56:18 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Tuesday 03 March 2015 13:40:50 Mauro Carvalho Chehab wrote:
> > Em Mon, 02 Mar 2015 20:52:41 +0000 Laurent Pinchart escreveu:
> > > On Mon Mar 02 2015 18:55:23 GMT+0200 (EET), Mauro Carvalho Chehab wrote:
> > >> Em Sun, 1 Feb 2015 12:12:33 +0100 (CET) Guennadi Liakhovetski escreveu:
> > >>> 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>
> > >>> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>> ---
> > >>> 
> > >>> v4: sizeof(*clk) :)
> > >>> 
> > >>>  drivers/media/v4l2-core/v4l2-clk.c | 48 ++++++++++++++++++++++++++---
> > >>>  include/media/v4l2-clk.h           |  2 ++
> > >>>  2 files changed, 47 insertions(+), 3 deletions(-)
> > >>> 
> > >>> diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> > >>> b/drivers/media/v4l2-core/v4l2-clk.c index 3ff0b00..9f8cb20 100644
> > >>> --- a/drivers/media/v4l2-core/v4l2-clk.c
> > >>> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> 
> [snip]
> 
> > >>> @@ -37,6 +38,21 @@ static struct v4l2_clk *v4l2_clk_find(const char
> > >>> *dev_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 (PTR_ERR(ccf_clk) == -EPROBE_DEFER)
> > >>> +		return ERR_PTR(-EPROBE_DEFER);
> > >> 
> > >> Why not do just:
> > >> 		return ccf_clk;
> > > 
> > > I find the explicit error slightly more readable, but that's a matter of
> > > taste.
> >
> > Well, return(ccf_clk) will likely produce a smaller instruction code
> > than return (long).
> 
> Not if the compiler is smart :-)
> 
> > >>> +
> > >>> +	if (!IS_ERR_OR_NULL(ccf_clk)) {
> > >>> +		clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> > >>> +		if (!clk) {
> > >>> +			clk_put(ccf_clk);
> > >>> +			return ERR_PTR(-ENOMEM);
> > >>> +		}
> > >>> +		clk->clk = ccf_clk;
> > >>> +
> > >>> +		return clk;
> > >>> +	}
> > >> 
> > >> The error condition here looks a little weird to me. I mean, if the
> > >> CCF clock returns an error, shouldn't it fail instead of silently
> > >> run some logic to find another clock source? Isn't it risky on getting
> > >> a wrong value?
> > > 
> > > The idea is that, in the long term, everything should use CCF directly.
> > > However, we have clock providers on platforms where CCF isn't avalaible.
> > > V4L2 clock has been introduced  as a  single API usable by V4L2 clock
> > > users allowing them to retrieve and use clocks regardless of whether the
> > > provider uses CCF or not. Internally it first tries CCF, and then falls
> > > back to the non-CCF implementation in case of failure.
> >
> > Yeah, I got that the non-CCF is a fallback code, to be used on
> > platforms that CCF isn't available.
> > 
> > However, the above code doesn't seem to look if CCF is available
> > or not. Instead, it assumes that *all* error codes, or even NULL,
> > means that CCF isn't available.
> > 
> > Shouldn't it be, instead, either waiting for NULL or for some
> > specific error code, in order to:
> > - return the error code, if CCF is available but getting
> >   the clock failed;
> > - run the backward-compat code when CCF is not available.
> 
> Isn't that pretty much what the code is doing ? If we get a -EPROBE_DEFER 
> error from CCF meaning that the clock is known but not registered yet we 
> return it. Otherwise, if the clock is unknown to CCF, or if CCF is disabled, 
> we fall back.

I didn't check the CCF code, but couldn't it return error codes like
ENOMEM? What are all the error codes it can return ATM? What, among
them, can happen when CCF is available?

Also, as the CCF code can be changed, if the intent behavior is to only
allow EPROBE_DEFER or NULL, if the there is support for CCF, then you
need to have an explicit comment there to avoid that any newer patches
to add different error codes.

IMHO, it seems a way better to define a single error code to be
returned when the platform doesn't support CCF (like -ENOTSUPP),
and calling the fallback code only in this case. Something like:

	if (PTR_ERR(ccf_clk) != -ENOTSUPP)
		return ccf_clk;

	/* CCF is not supported. Fall back to the old way */	
	k = kzalloc(sizeof(*clk), GFP_KERNEL);
	if (!clk) {
		clk_put(ccf_clk);
		return ERR_PTR(-ENOMEM);
	}
	clk->clk = ccf_clk;
	return clk;

Regards,
Mauro

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

[Laurent Pinchart]

Hi Mauro,

On Tuesday 03 March 2015 20:21:29 Mauro Carvalho Chehab wrote:
> Em Wed, 04 Mar 2015 00:56:18 +0200 Laurent Pinchart escreveu:
> > On Tuesday 03 March 2015 13:40:50 Mauro Carvalho Chehab wrote:
> >> Em Mon, 02 Mar 2015 20:52:41 +0000 Laurent Pinchart escreveu:
> >>> On Mon Mar 02 2015 18:55:23 GMT+0200 (EET), Mauro Carvalho Chehab wrote:
> >>>> Em Sun, 1 Feb 2015 12:12:33 +0100 (CET) Guennadi Liakhovetski escreveu:
> >>>>> 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>
> >>>>> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>> ---
> >>>>> 
> >>>>> v4: sizeof(*clk) :)
> >>>>> 
> >>>>>  drivers/media/v4l2-core/v4l2-clk.c | 48 ++++++++++++++++++++++++---
> >>>>>  include/media/v4l2-clk.h           |  2 ++
> >>>>>  2 files changed, 47 insertions(+), 3 deletions(-)
> >>>>> 
> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-clk.c
> >>>>> b/drivers/media/v4l2-core/v4l2-clk.c index 3ff0b00..9f8cb20 100644
> >>>>> --- a/drivers/media/v4l2-core/v4l2-clk.c
> >>>>> +++ b/drivers/media/v4l2-core/v4l2-clk.c
> > 
> > [snip]
> > 
> >>>>> @@ -37,6 +38,21 @@ static struct v4l2_clk *v4l2_clk_find(const char
> >>>>> *dev_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 (PTR_ERR(ccf_clk) == -EPROBE_DEFER)
> >>>>> +		return ERR_PTR(-EPROBE_DEFER);
> >>>> 
> >>>> Why not do just:
> >>>> 		return ccf_clk;
> >>> 
> >>> I find the explicit error slightly more readable, but that's a matter
> >>> of taste.
> >> 
> >> Well, return(ccf_clk) will likely produce a smaller instruction code
> >> than return (long).
> > 
> > Not if the compiler is smart :-)
> > 
> >>>>> +
> >>>>> +	if (!IS_ERR_OR_NULL(ccf_clk)) {
> >>>>> +		clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> >>>>> +		if (!clk) {
> >>>>> +			clk_put(ccf_clk);
> >>>>> +			return ERR_PTR(-ENOMEM);
> >>>>> +		}
> >>>>> +		clk->clk = ccf_clk;
> >>>>> +
> >>>>> +		return clk;
> >>>>> +	}
> >>>> 
> >>>> The error condition here looks a little weird to me. I mean, if the
> >>>> CCF clock returns an error, shouldn't it fail instead of silently
> >>>> run some logic to find another clock source? Isn't it risky on
> >>>> getting a wrong value?
> >>> 
> >>> The idea is that, in the long term, everything should use CCF
> >>> directly. However, we have clock providers on platforms where CCF
> >>> isn't avalaible. V4L2 clock has been introduced  as a  single API
> >>> usable by V4L2 clock users allowing them to retrieve and use clocks
> >>> regardless of whether the provider uses CCF or not. Internally it
> >>> first tries CCF, and then falls back to the non-CCF implementation in
> >>> case of failure.
> >> 
> >> Yeah, I got that the non-CCF is a fallback code, to be used on
> >> platforms that CCF isn't available.
> >> 
> >> However, the above code doesn't seem to look if CCF is available
> >> or not. Instead, it assumes that *all* error codes, or even NULL,
> >> means that CCF isn't available.
> >> 
> >> Shouldn't it be, instead, either waiting for NULL or for some
> >> specific error code, in order to:
> >>
> >> - return the error code, if CCF is available but getting
> >>   the clock failed;
> >> 
> >> - run the backward-compat code when CCF is not available.
> > 
> > Isn't that pretty much what the code is doing ? If we get a -EPROBE_DEFER
> > error from CCF meaning that the clock is known but not registered yet we
> > return it. Otherwise, if the clock is unknown to CCF, or if CCF is
> > disabled, we fall back.
> 
> I didn't check the CCF code, but couldn't it return error codes like
> ENOMEM? What are all the error codes it can return ATM? What, among
> them, can happen when CCF is available?
> 
> Also, as the CCF code can be changed, if the intent behavior is to only
> allow EPROBE_DEFER or NULL, if the there is support for CCF, then you
> need to have an explicit comment there to avoid that any newer patches
> to add different error codes.
> 
> IMHO, it seems a way better to define a single error code to be
> returned when the platform doesn't support CCF (like -ENOTSUPP),

But that's not how CCF works :-)

Regardless of that, note that the clk_get() call is part of the Linux clock 
API, of which CCF is one implementation. clk_get() is implemented by custom 
architecture code on platforms where CCF isn't used.

The idea of this patch is to

- return the platform clock if available (clk_get() returns a non-NULL, non-
error pointer)

- defer probing if the platform clock requests so

- fall back to the internal clocks list

I don't see a need for something else.

> and calling the fallback code only in this case. Something like:
> 
> 	if (PTR_ERR(ccf_clk) != -ENOTSUPP)
> 		return ccf_clk;
> 
> 	/* CCF is not supported. Fall back to the old way */
> 	k = kzalloc(sizeof(*clk), GFP_KERNEL);
> 	if (!clk) {
> 		clk_put(ccf_clk);
> 		return ERR_PTR(-ENOMEM);
> 	}
> 	clk->clk = ccf_clk;
> 	return clk;


-- 
Regards,

Laurent Pinchart

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

[Guennadi Liakhovetski]

Hi Mauro,

On Mon, 2 Mar 2015, Mauro Carvalho Chehab wrote:

> Em Sun, 1 Feb 2015 12:12:33 +0100 (CET)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> escreveu:
> 
> > 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>
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> > v4: sizeof(*clk) :)
> > 
> >  drivers/media/v4l2-core/v4l2-clk.c | 48 +++++++++++++++++++++++++++++++++++---
> >  include/media/v4l2-clk.h           |  2 ++
> >  2 files changed, 47 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-clk.c b/drivers/media/v4l2-core/v4l2-clk.c
> > index 3ff0b00..9f8cb20 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>
> > @@ -37,6 +38,21 @@ static struct v4l2_clk *v4l2_clk_find(const char *dev_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 (PTR_ERR(ccf_clk) == -EPROBE_DEFER)
> > +		return ERR_PTR(-EPROBE_DEFER);
> 
> Why not do just:
> 		return ccf_clk;

I would prefer that shorter form too, but the function returns "struct 
v4l2_clk *" and ccf_clk is "struct clk *"

Thanks
Guennadi