Re: [PATCH v2 2/2] V4L2: add CCF support to the v4l2_clk API
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.liakhovet...@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.liakhovet...@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;
+
Re: [PATCH v2 2/2] V4L2: add CCF support to the v4l2_clk API
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.liakhovet...@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.liakhovet...@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 {
Re: [PATCH v2 2/2] V4L2: add CCF support to the v4l2_clk API
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.liakhovet...@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.liakhovet...@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;
};
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] V4L2: add CCF support to the v4l2_clk API
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.liakhovet...@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.liakhovet...@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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to
Re: [PATCH v2 2/2] V4L2: add CCF support to the v4l2_clk API
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.liakhovet...@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.liakhovet...@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
+++
Re: [PATCH v2 2/2] V4L2: add CCF support to the v4l2_clk API
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.liakhovet...@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.liakhovet...@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) {
+
[PATCH v2 2/2] V4L2: add CCF support to the v4l2_clk API
From aeaee56e04d023f3a019d2595ef5128015acdb06 Mon Sep 17 00:00:00 2001
From: Guennadi Liakhovetski g.liakhovet...@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.liakhovet...@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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
