On 12/02/2013 01:52 AM, Thierry Reding wrote:
> On Sun, Dec 01, 2013 at 12:05:44PM -0700, Stephen Warren wrote:
>> On 11/29/2013 04:49 AM, Thierry Reding wrote:
>>> On Fri, Nov 15, 2013 at 01:53:56PM -0700, Stephen Warren wrote:
>>> [...]
>>>> @@ -60,6 +81,12 @@ of the following host1x client modules: -
>>>> compatible: "nvidia,tegra<chip>-dc" - reg: Physical base
>>>> address and length of the controller's registers. -
>>>> interrupts: The interrupt outputs from the controller. + -
>>>> clocks : Must contain an entry for each entry in clock-names.
>>>> + See ../clocks/clock-bindings.txt for details. + -
>>>> clock-names : Must include the following entries: + -
>>>> disp1 or disp2 (depending on the controller instance)
>>>
>>> I'm not sure if this makes sense. The name could be the same
>>> independent of which controller uses it. If it isn't then the
>>> driver would need additional code to find out which instance it
>>> is and construct a dynamic string.
>>>
>>> Any objection to just make this entry "disp", or "dc"?
>>
>> This patch simply documents the binding that the various drivers
>> already require and/or whatever is already in the DT files if
>> there are any clocks the drivers don't currently use. I did
>> consider fixing up all the current usage to actually be sane, but
>> that would require even more driver changes (in addition to those
>> required for the reset framework patches).
>
> Okay, I understand. I still think we should change the usage for
> this particular use-case subsequently. In retrospect the entry in
> clock-names wasn't thought out very well. It seems like the reason
> for using disp1 and disp2 respectively was so that it would match
> the system-wide clock name, rather than the clock's label within
> the display controller's context.
>
> Just to clarify what I mean, if we stick to the above, then we'll
> need to add code to the driver along the lines of:
>
> char clock_name[6];
>
> if (regs->start == 0x54200000) index = 1; else index = 2;
>
> sprintf(clock_name, "disp%u", index);
>
> clk = devm_clk_get(&pdev->dev, clock_name);
>
> rather than the much more simple and elegant:
>
> clk = devm_clk_get(&pdev->dev, "disp");
>
> The whole purpose of the clock consumer ID is to be generic and as
> such independent of the specific IP block or instance thereof.
I think if the code needs this clock, I'd be tempted to do the following:
clk = clk_get(dev, "disp1");
if (IS_ERR(clk) && PTR_ERR(clk) != -EPROBE_DEFERRED)
clk = clk_get(dev, "disp2");
if (IS_ERR(clk))
return PTR_ERR(clk);
That avoids having to hard-code IP block base addresses and construct
clock names at run-time.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
