[PATCH] mt9v032: Use the common clock framework

[PATCH] mt9v032: Use the common clock framework

[Laurent Pinchart]

Configure the device external clock using the common clock framework
instead of a board code callback function.

Signed-off-by: Laurent Pinchart <laurent.pinchart@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.
  */
 
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/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;
 };
-- 
Regards,

Laurent Pinchart

Re: [PATCH] mt9v032: Use the common clock framework

[Sylwester Nawrocki]

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 Pinchart<laurent.pinchart@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.
>    */
>
> +#include<linux/clk.h>
>   #include<linux/delay.h>
>   #include<linux/i2c.h>
>   #include<linux/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

Re: [PATCH] mt9v032: Use the common clock framework

[Laurent Pinchart]

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 Pinchart<laurent.pinchart@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.
> >    */
> > 
> > +#include<linux/clk.h>
> >   #include<linux/delay.h>
> >   #include<linux/i2c.h>
> >   #include<linux/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

Re: [PATCH] mt9v032: Use the common clock framework

[Sylwester Nawrocki]

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 Pinchart<laurent.pinchart@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.
>>>     */
>>>
>>> +#include<linux/clk.h>
>>>    #include<linux/delay.h>
>>>    #include<linux/i2c.h>
>>>    #include<linux/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

Re: [PATCH] mt9v032: Use the common clock framework

[Laurent Pinchart]

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 Pinchart<laurent.pinchart@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.
> >>>     */
> >>> 
> >>> +#include<linux/clk.h>
> >>>    #include<linux/delay.h>
> >>>    #include<linux/i2c.h>
> >>>    #include<linux/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

Re: [PATCH] mt9v032: Use the common clock framework

[Sylwester Nawrocki]

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 Pinchart<laurent.pinchart@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.
>>>>>     */
>>>>>
>>>>> +#include<linux/clk.h>
>>>>>    #include<linux/delay.h>
>>>>>    #include<linux/i2c.h>
>>>>>    #include<linux/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

Re: [PATCH] mt9v032: Use the common clock framework

[Laurent Pinchart]

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 Pinchart<laurent.pinchart@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