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@xxxxxxxxxxxxxxxx>
>>>>> ---
>>>>>
>>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
