Re: [PATCH] omapdss: use devm_clk_get()

[Archit Taneja <archit@ti.com>]

On Wednesday 10 April 2013 02:58 PM, Archit Taneja wrote:
> On Wednesday 10 April 2013 02:49 PM, Tomi Valkeinen wrote:
>> On 2013-04-10 11:37, Archit Taneja wrote:
>>> On Wednesday 10 April 2013 01:43 PM, Tomi Valkeinen wrote:
>>>> On 2013-04-08 11:55, Archit Taneja wrote:
>>>>> Use devm_clk_get() instead of clk_get() for dss, and for outputs hdmi
>>>>> and venc.
>>>>> This reduces reduces code and simplifies error handling.
>>>>>
>>>>> Signed-off-by: Archit Taneja <archit@ti.com>
>>>>> ---
>>>>>    drivers/video/omap2/dss/dss.c  |   18 +++---------------
>>>>>    drivers/video/omap2/dss/hdmi.c |   16 ++--------------
>>>>>    drivers/video/omap2/dss/venc.c |   10 +---------
>>>>>    3 files changed, 6 insertions(+), 38 deletions(-)
>>>>>
>>>>> diff --git a/drivers/video/omap2/dss/dss.c
>>>>> b/drivers/video/omap2/dss/dss.c
>>>>> index 054c2a2..645b3bc 100644
>>>>> --- a/drivers/video/omap2/dss/dss.c
>>>>> +++ b/drivers/video/omap2/dss/dss.c
>>>>> @@ -767,13 +767,11 @@ int dss_dpi_select_source(enum omap_channel
>>>>> channel)
>>>>>    static int dss_get_clocks(void)
>>>>>    {
>>>>>        struct clk *clk;
>>>>> -    int r;
>>>>>
>>>>> -    clk = clk_get(&dss.pdev->dev, "fck");
>>>>> +    clk = devm_clk_get(&dss.pdev->dev, "fck");
>>>>>        if (IS_ERR(clk)) {
>>>>>            DSSERR("can't get clock fck\n");
>>>>> -        r = PTR_ERR(clk);
>>>>> -        goto err;
>>>>> +        return PTR_ERR(clk);
>>>>>        }
>>>>>
>>>>>        dss.dss_clk = clk;
>>>>> @@ -782,8 +780,7 @@ static int dss_get_clocks(void)
>>>>>            clk = clk_get(NULL, dss.feat->clk_name);
>>>>>            if (IS_ERR(clk)) {
>>>>>                DSSERR("Failed to get %s\n", dss.feat->clk_name);
>>>>> -            r = PTR_ERR(clk);
>>>>> -            goto err;
>>>>> +            return PTR_ERR(clk);
>>>>>            }
>>>>>        } else {
>>>>>            clk = NULL;
>>>>> @@ -792,21 +789,12 @@ static int dss_get_clocks(void)
>>>>>        dss.dpll4_m4_ck = clk;
>>>>>
>>>>>        return 0;
>>>>> -
>>>>> -err:
>>>>> -    if (dss.dss_clk)
>>>>> -        clk_put(dss.dss_clk);
>>>>> -    if (dss.dpll4_m4_ck)
>>>>> -        clk_put(dss.dpll4_m4_ck);
>>>>> -
>>>>> -    return r;
>>>>>    }
>>>>
>>>> Why didn't you use devm_clk_get for the dpll4_m4_ck clock also?
>>>
>>> clk_get of dpll4_m4_ck isn't tied to a device:
>>>
>>>      clk = clk_get(NULL, dss.feat->clk_name);
>>>
>>> We can't use devm_clk_get() if we don't tie it to a device, right?
>>
>> Ah, true.
>>
>>> I think the dss.dss_clk clock above is same as the dpll4_m4_ck for all
>>> OMAPs. We could probably remove the dpll4_m4_clk all together, and use
>>> dss_clk to get the rate of DSS_FCK coming from PRCM.
>>
>> Hmm, no, if I recall right, dpll4_m4_clk is the parent of dss clock (or
>> parent of the parent of...), and the dss_fck_multiplier affects the
>> resulting dss fck. Or something like that. It's a bit messy, especially
>> as we can't just use the dss fck to change the rate, we need to change
>> the rate at the parent for some reason.
>
> Ah ok, I remember it now. One of the DPLL's divider's output is DSS_FCK.
> We get the parent's rate and iterate through all the possible hsdivider
> values to get the closest pixel clock. We don't/can't change
> dpll4_m4_clk as such, we just need to know it's rate for our calculations.
>
> So yes, we need dpll4_m4_clk, we just need to rename it to a better
> thing. Like dss_clk_parent.
>
> Anyway,

Sorry, hit the send button before completing the message. I wanted to 
add that we could push this patch as it is.

Archit