On Tue, Dec 03, 2013 at 11:31:00AM -0700, Stephen Warren wrote: > 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. I think perhaps we're getting our wires crossed. What I've been trying to say is that I think the binding should define the first clock to be named simply "disp". The reason why I think "disp" is a better choice than "disp1" and "disp2" is that it merely encodes the purpose of the clock for the display controller, and doesn't contain knowledge about the particular instance of the display controller. That's analogous to I2C or SPI nodes where the clock isn't named "i2c1", "i2c2", ... or "spi1", "spi2", ... but simply "i2c" or "spi" respectively. I know that existing DTS files use "disp1" and "disp2", respectively, but I think that was a wrong choice back at the time and therefore I suggest that we change it while we still can (DTS files are changing in 3.14 anyway because of the reset and DMA binding updates). Is that any clearer than what I was saying before? Thierry
Attachment:
pgpvSzbQQkE7S.pgp
Description: PGP signature
