Re: [PATCH] mt9v032: Use the common clock framework
Hi Sylwester, On Wednesday 07 August 2013 17:03:18 Sylwester Nawrocki wrote: On 07/26/2013 05:55 PM, Laurent Pinchart wrote: On Friday 26 July 2013 15:36:22 Sylwester Nawrocki wrote: On 07/26/2013 03:15 PM, Laurent Pinchart wrote: On Friday 26 July 2013 15:11:08 Sylwester Nawrocki wrote: On 07/05/2013 12:55 PM, Laurent Pinchart wrote: Configure the device external clock using the common clock framework instead of a board code callback function. Signed-off-by: Laurent Pinchartlaurent.pinch...@ideasonboard.com --- drivers/media/i2c/mt9v032.c | 16 ++-- include/media/mt9v032.h | 4 2 files changed, 10 insertions(+), 10 deletions(-) [snip] Do you rely on the fact that __clk_get()/__clk_put() doesn't get reference on the clock supplier module (to avoid locking modules in memory) ? I was planning on adding module_get()/module_put() inside __clk_get()/__clk_out() for the common clock API implementation. I'm currently relying on that, but I'm aware it's not a good idea. We need to find a solution to fix the problem in the context of the v4l2-async framework. There are few possible options I can see could be a resolution for that: 1. making the common clock API not getting reference on the clock supplier module; then dynamic clock deregistration needs to be handled, which has its own share of problems; 2. using sysfs unbind attribute of the subdev and/or the host drivers to release the circular references or create some additional sysfs entry for this - however requiring additional actions from user space to do something that worked before without them can be simply considered as an regression; Well, we've never had working DT support for those systems, so that's hardly a regression :-) 3. not keeping reference to a clock all times only when it is actively used, e.g. in subdev's s_power handler; this is in practice what v4l2-clk does, just would be coded using standard clk API calls; Any of these options isn't ideal but 3) seems to me most reasonable of them. For now it might be, but I'm not sure whether it would scale. We will likely have similar issues with other kind of resources in the future. I would be enclined to investigate the second option. I thought it would be better than using the v4l2-clk API, which is supposed to be a temporary measure only, for platforms that already have the common clock API support. I was wondering whether you don't need to also set the clock's frequency in this patch. I guess the sensor driver should at least call clk_get_rate() to see if current frequency is suitable for the device ? Yes indeed. I'm not sure how I've missed that. I'll fix it. -- Regards, Laurent Pinchart -- 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] mt9v032: Use the common clock framework
Hi Laurent,
On 07/26/2013 05:55 PM, Laurent Pinchart wrote:
On Friday 26 July 2013 15:36:22 Sylwester Nawrocki wrote:
On 07/26/2013 03:15 PM, Laurent Pinchart wrote:
On Friday 26 July 2013 15:11:08 Sylwester Nawrocki wrote:
On 07/05/2013 12:55 PM, Laurent Pinchart wrote:
Configure the device external clock using the common clock framework
instead of a board code callback function.
Signed-off-by: Laurent Pinchartlaurent.pinch...@ideasonboard.com
---
drivers/media/i2c/mt9v032.c | 16 ++--
include/media/mt9v032.h | 4
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
index 60c6f67..7b30640 100644
--- a/drivers/media/i2c/mt9v032.c
+++ b/drivers/media/i2c/mt9v032.c
@@ -12,6 +12,7 @@
* published by the Free Software Foundation.
*/
+#includelinux/clk.h
#includelinux/delay.h
#includelinux/i2c.h
#includelinux/log2.h
@@ -135,6 +136,8 @@ struct mt9v032 {
struct mutex power_lock;
int power_count;
+ struct clk *clk;
+
struct mt9v032_platform_data *pdata;
u32 sysclk;
@@ -219,10 +222,8 @@ static int mt9v032_power_on(struct mt9v032
*mt9v032)
struct i2c_client *client =
v4l2_get_subdevdata(mt9v032-subdev);
int ret;
- if (mt9v032-pdata-set_clock) {
- mt9v032-pdata-set_clock(mt9v032-subdev, mt9v032-sysclk);
- udelay(1);
- }
+ clk_prepare_enable(mt9v032-clk);
+ udelay(1);
/* Reset the chip and stop data read out */
ret = mt9v032_write(client, MT9V032_RESET, 1);
@@ -238,8 +239,7 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
static void mt9v032_power_off(struct mt9v032 *mt9v032)
{
- if (mt9v032-pdata-set_clock)
- mt9v032-pdata-set_clock(mt9v032-subdev, 0);
+ clk_disable_unprepare(mt9v032-clk);
}
static int __mt9v032_set_power(struct mt9v032 *mt9v032, bool on)
@@ -748,6 +748,10 @@ static int mt9v032_probe(struct i2c_client *client,
if (!mt9v032)
return -ENOMEM;
+ mt9v032-clk = devm_clk_get(client-dev, NULL);
+ if (IS_ERR(mt9v032-clk))
+ return PTR_ERR(mt9v032-clk);
+
mutex_init(mt9v032-power_lock);
mt9v032-pdata = pdata;
diff --git a/include/media/mt9v032.h b/include/media/mt9v032.h
index 78fd39e..12175a6 100644
--- a/include/media/mt9v032.h
+++ b/include/media/mt9v032.h
@@ -1,13 +1,9 @@
#ifndef _MEDIA_MT9V032_H
#define _MEDIA_MT9V032_H
-struct v4l2_subdev;
-
struct mt9v032_platform_data {
unsigned int clk_pol:1;
- void (*set_clock)(struct v4l2_subdev *subdev, unsigned int rate);
-
const s64 *link_freqs;
s64 link_def_freq;
};
Is there clk_put() somewhere in this patch ? I would expect it somewhere
around driver remove() callback, but can't see it. :-/
There's *devm_*clk_get() instead :-)
Ah, I knew I must have been forgetting or overlooking something! ;)
Do you rely on the fact that __clk_get()/__clk_put() doesn't get reference
on the clock supplier module (to avoid locking modules in memory) ? I was
planning on adding module_get()/module_put() inside __clk_get()/__clk_out()
for the common clock API implementation.
I'm currently relying on that, but I'm aware it's not a good idea. We need to
find a solution to fix the problem in the context of the v4l2-async framework.
There are few possible options I can see could be a resolution for that:
1. making the common clock API not getting reference on the clock supplier
module; then dynamic clock deregistration needs to be handled, which has
its own share of problems;
2. using sysfs unbind attribute of the subdev and/or the host drivers to
release the circular references or create some additional sysfs entry
for this - however requiring additional actions from user space to do
something that worked before without them can be simply considered as
an regression;
3. not keeping reference to a clock all times only when it is actively
used, e.g. in subdev's s_power handler; this is in practice what
v4l2-clk does, just would be coded using standard clk API calls;
Any of these options isn't ideal but 3) seems to me most reasonable of
them. I thought it would be better than using the v4l2-clk API, which is
supposed to be a temporary measure only, for platforms that already have
the common clock API support.
I was wondering whether you don't need to also set the clock's frequency
in this patch. I guess the sensor driver should at least call
clk_get_rate() to see if current frequency is suitable for the device ?
Regards,
Sylwester
--
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] mt9v032: Use the common clock framework
Hi Laurent,
On 07/05/2013 12:55 PM, Laurent Pinchart wrote:
Configure the device external clock using the common clock framework
instead of a board code callback function.
Signed-off-by: Laurent Pinchartlaurent.pinch...@ideasonboard.com
---
drivers/media/i2c/mt9v032.c | 16 ++--
include/media/mt9v032.h | 4
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
index 60c6f67..7b30640 100644
--- a/drivers/media/i2c/mt9v032.c
+++ b/drivers/media/i2c/mt9v032.c
@@ -12,6 +12,7 @@
* published by the Free Software Foundation.
*/
+#includelinux/clk.h
#includelinux/delay.h
#includelinux/i2c.h
#includelinux/log2.h
@@ -135,6 +136,8 @@ struct mt9v032 {
struct mutex power_lock;
int power_count;
+ struct clk *clk;
+
struct mt9v032_platform_data *pdata;
u32 sysclk;
@@ -219,10 +222,8 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
struct i2c_client *client = v4l2_get_subdevdata(mt9v032-subdev);
int ret;
- if (mt9v032-pdata-set_clock) {
- mt9v032-pdata-set_clock(mt9v032-subdev, mt9v032-sysclk);
- udelay(1);
- }
+ clk_prepare_enable(mt9v032-clk);
+ udelay(1);
/* Reset the chip and stop data read out */
ret = mt9v032_write(client, MT9V032_RESET, 1);
@@ -238,8 +239,7 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
static void mt9v032_power_off(struct mt9v032 *mt9v032)
{
- if (mt9v032-pdata-set_clock)
- mt9v032-pdata-set_clock(mt9v032-subdev, 0);
+ clk_disable_unprepare(mt9v032-clk);
}
static int __mt9v032_set_power(struct mt9v032 *mt9v032, bool on)
@@ -748,6 +748,10 @@ static int mt9v032_probe(struct i2c_client *client,
if (!mt9v032)
return -ENOMEM;
+ mt9v032-clk = devm_clk_get(client-dev, NULL);
+ if (IS_ERR(mt9v032-clk))
+ return PTR_ERR(mt9v032-clk);
+
mutex_init(mt9v032-power_lock);
mt9v032-pdata = pdata;
diff --git a/include/media/mt9v032.h b/include/media/mt9v032.h
index 78fd39e..12175a6 100644
--- a/include/media/mt9v032.h
+++ b/include/media/mt9v032.h
@@ -1,13 +1,9 @@
#ifndef _MEDIA_MT9V032_H
#define _MEDIA_MT9V032_H
-struct v4l2_subdev;
-
struct mt9v032_platform_data {
unsigned int clk_pol:1;
- void (*set_clock)(struct v4l2_subdev *subdev, unsigned int rate);
-
const s64 *link_freqs;
s64 link_def_freq;
};
Is there clk_put() somewhere in this patch ? I would expect it somewhere
around driver remove() callback, but can't see it. :-/
Regards,
Sylwester
--
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] mt9v032: Use the common clock framework
Hi Sylwester,
On Friday 26 July 2013 15:11:08 Sylwester Nawrocki wrote:
On 07/05/2013 12:55 PM, Laurent Pinchart wrote:
Configure the device external clock using the common clock framework
instead of a board code callback function.
Signed-off-by: Laurent Pinchartlaurent.pinch...@ideasonboard.com
---
drivers/media/i2c/mt9v032.c | 16 ++--
include/media/mt9v032.h | 4
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
index 60c6f67..7b30640 100644
--- a/drivers/media/i2c/mt9v032.c
+++ b/drivers/media/i2c/mt9v032.c
@@ -12,6 +12,7 @@
* published by the Free Software Foundation.
*/
+#includelinux/clk.h
#includelinux/delay.h
#includelinux/i2c.h
#includelinux/log2.h
@@ -135,6 +136,8 @@ struct mt9v032 {
struct mutex power_lock;
int power_count;
+ struct clk *clk;
+
struct mt9v032_platform_data *pdata;
u32 sysclk;
@@ -219,10 +222,8 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
struct i2c_client *client = v4l2_get_subdevdata(mt9v032-subdev);
int ret;
- if (mt9v032-pdata-set_clock) {
- mt9v032-pdata-set_clock(mt9v032-subdev, mt9v032-sysclk);
- udelay(1);
- }
+ clk_prepare_enable(mt9v032-clk);
+ udelay(1);
/* Reset the chip and stop data read out */
ret = mt9v032_write(client, MT9V032_RESET, 1);
@@ -238,8 +239,7 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
static void mt9v032_power_off(struct mt9v032 *mt9v032)
{
- if (mt9v032-pdata-set_clock)
- mt9v032-pdata-set_clock(mt9v032-subdev, 0);
+ clk_disable_unprepare(mt9v032-clk);
}
static int __mt9v032_set_power(struct mt9v032 *mt9v032, bool on)
@@ -748,6 +748,10 @@ static int mt9v032_probe(struct i2c_client *client,
if (!mt9v032)
return -ENOMEM;
+ mt9v032-clk = devm_clk_get(client-dev, NULL);
+ if (IS_ERR(mt9v032-clk))
+ return PTR_ERR(mt9v032-clk);
+
mutex_init(mt9v032-power_lock);
mt9v032-pdata = pdata;
diff --git a/include/media/mt9v032.h b/include/media/mt9v032.h
index 78fd39e..12175a6 100644
--- a/include/media/mt9v032.h
+++ b/include/media/mt9v032.h
@@ -1,13 +1,9 @@
#ifndef _MEDIA_MT9V032_H
#define _MEDIA_MT9V032_H
-struct v4l2_subdev;
-
struct mt9v032_platform_data {
unsigned int clk_pol:1;
- void (*set_clock)(struct v4l2_subdev *subdev, unsigned int rate);
-
const s64 *link_freqs;
s64 link_def_freq;
};
Is there clk_put() somewhere in this patch ? I would expect it somewhere
around driver remove() callback, but can't see it. :-/
There's *devm_*clk_get() instead :-)
--
Regards,
Laurent Pinchart
--
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] mt9v032: Use the common clock framework
Hi Laurent,
On 07/26/2013 03:15 PM, Laurent Pinchart wrote:
On Friday 26 July 2013 15:11:08 Sylwester Nawrocki wrote:
On 07/05/2013 12:55 PM, Laurent Pinchart wrote:
Configure the device external clock using the common clock framework
instead of a board code callback function.
Signed-off-by: Laurent Pinchartlaurent.pinch...@ideasonboard.com
---
drivers/media/i2c/mt9v032.c | 16 ++--
include/media/mt9v032.h | 4
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
index 60c6f67..7b30640 100644
--- a/drivers/media/i2c/mt9v032.c
+++ b/drivers/media/i2c/mt9v032.c
@@ -12,6 +12,7 @@
* published by the Free Software Foundation.
*/
+#includelinux/clk.h
#includelinux/delay.h
#includelinux/i2c.h
#includelinux/log2.h
@@ -135,6 +136,8 @@ struct mt9v032 {
struct mutex power_lock;
int power_count;
+ struct clk *clk;
+
struct mt9v032_platform_data *pdata;
u32 sysclk;
@@ -219,10 +222,8 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
struct i2c_client *client = v4l2_get_subdevdata(mt9v032-subdev);
int ret;
- if (mt9v032-pdata-set_clock) {
- mt9v032-pdata-set_clock(mt9v032-subdev, mt9v032-sysclk);
- udelay(1);
- }
+ clk_prepare_enable(mt9v032-clk);
+ udelay(1);
/* Reset the chip and stop data read out */
ret = mt9v032_write(client, MT9V032_RESET, 1);
@@ -238,8 +239,7 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
static void mt9v032_power_off(struct mt9v032 *mt9v032)
{
- if (mt9v032-pdata-set_clock)
- mt9v032-pdata-set_clock(mt9v032-subdev, 0);
+ clk_disable_unprepare(mt9v032-clk);
}
static int __mt9v032_set_power(struct mt9v032 *mt9v032, bool on)
@@ -748,6 +748,10 @@ static int mt9v032_probe(struct i2c_client *client,
if (!mt9v032)
return -ENOMEM;
+ mt9v032-clk = devm_clk_get(client-dev, NULL);
+ if (IS_ERR(mt9v032-clk))
+ return PTR_ERR(mt9v032-clk);
+
mutex_init(mt9v032-power_lock);
mt9v032-pdata = pdata;
diff --git a/include/media/mt9v032.h b/include/media/mt9v032.h
index 78fd39e..12175a6 100644
--- a/include/media/mt9v032.h
+++ b/include/media/mt9v032.h
@@ -1,13 +1,9 @@
#ifndef _MEDIA_MT9V032_H
#define _MEDIA_MT9V032_H
-struct v4l2_subdev;
-
struct mt9v032_platform_data {
unsigned int clk_pol:1;
- void (*set_clock)(struct v4l2_subdev *subdev, unsigned int rate);
-
const s64 *link_freqs;
s64 link_def_freq;
};
Is there clk_put() somewhere in this patch ? I would expect it somewhere
around driver remove() callback, but can't see it. :-/
There's *devm_*clk_get() instead :-)
Ah, I knew I must have been forgetting or overlooking something! ;)
Do you rely on the fact that __clk_get()/__clk_put() doesn't get reference
on the clock supplier module (to avoid locking modules in memory) ? I was
planning on adding module_get()/module_put() inside __clk_get()/__clk_out()
for the common clock API implementation.
Regards,
Sylwester
--
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] mt9v032: Use the common clock framework
Hi Sylwester,
On Friday 26 July 2013 15:36:22 Sylwester Nawrocki wrote:
On 07/26/2013 03:15 PM, Laurent Pinchart wrote:
On Friday 26 July 2013 15:11:08 Sylwester Nawrocki wrote:
On 07/05/2013 12:55 PM, Laurent Pinchart wrote:
Configure the device external clock using the common clock framework
instead of a board code callback function.
Signed-off-by: Laurent Pinchartlaurent.pinch...@ideasonboard.com
---
drivers/media/i2c/mt9v032.c | 16 ++--
include/media/mt9v032.h | 4
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
index 60c6f67..7b30640 100644
--- a/drivers/media/i2c/mt9v032.c
+++ b/drivers/media/i2c/mt9v032.c
@@ -12,6 +12,7 @@
* published by the Free Software Foundation.
*/
+#includelinux/clk.h
#includelinux/delay.h
#includelinux/i2c.h
#includelinux/log2.h
@@ -135,6 +136,8 @@ struct mt9v032 {
struct mutex power_lock;
int power_count;
+ struct clk *clk;
+
struct mt9v032_platform_data *pdata;
u32 sysclk;
@@ -219,10 +222,8 @@ static int mt9v032_power_on(struct mt9v032
*mt9v032)
struct i2c_client *client =
v4l2_get_subdevdata(mt9v032-subdev);
int ret;
- if (mt9v032-pdata-set_clock) {
- mt9v032-pdata-set_clock(mt9v032-subdev, mt9v032-sysclk);
- udelay(1);
- }
+ clk_prepare_enable(mt9v032-clk);
+ udelay(1);
/* Reset the chip and stop data read out */
ret = mt9v032_write(client, MT9V032_RESET, 1);
@@ -238,8 +239,7 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
static void mt9v032_power_off(struct mt9v032 *mt9v032)
{
- if (mt9v032-pdata-set_clock)
- mt9v032-pdata-set_clock(mt9v032-subdev, 0);
+ clk_disable_unprepare(mt9v032-clk);
}
static int __mt9v032_set_power(struct mt9v032 *mt9v032, bool on)
@@ -748,6 +748,10 @@ static int mt9v032_probe(struct i2c_client *client,
if (!mt9v032)
return -ENOMEM;
+ mt9v032-clk = devm_clk_get(client-dev, NULL);
+ if (IS_ERR(mt9v032-clk))
+ return PTR_ERR(mt9v032-clk);
+
mutex_init(mt9v032-power_lock);
mt9v032-pdata = pdata;
diff --git a/include/media/mt9v032.h b/include/media/mt9v032.h
index 78fd39e..12175a6 100644
--- a/include/media/mt9v032.h
+++ b/include/media/mt9v032.h
@@ -1,13 +1,9 @@
#ifndef _MEDIA_MT9V032_H
#define _MEDIA_MT9V032_H
-struct v4l2_subdev;
-
struct mt9v032_platform_data {
unsigned int clk_pol:1;
- void (*set_clock)(struct v4l2_subdev *subdev, unsigned int rate);
-
const s64 *link_freqs;
s64 link_def_freq;
};
Is there clk_put() somewhere in this patch ? I would expect it somewhere
around driver remove() callback, but can't see it. :-/
There's *devm_*clk_get() instead :-)
Ah, I knew I must have been forgetting or overlooking something! ;)
Do you rely on the fact that __clk_get()/__clk_put() doesn't get reference
on the clock supplier module (to avoid locking modules in memory) ? I was
planning on adding module_get()/module_put() inside __clk_get()/__clk_out()
for the common clock API implementation.
I'm currently relying on that, but I'm aware it's not a good idea. We need to
find a solution to fix the problem in the context of the v4l2-async framework.
--
Regards,
Laurent Pinchart
--
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
