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.

> >> diff --git
> >> a/Documentation/devicetree/bindings/spi/nvidia,tegra114-spi.txt
> >> b/Documentation/devicetree/bindings/spi/nvidia,tegra114-spi.txt 
> >> index 91ff771c7e77..d4f2d534934b 100644 ---
> >> a/Documentation/devicetree/bindings/spi/nvidia,tegra114-spi.txt 
> >> +++
> >> b/Documentation/devicetree/bindings/spi/nvidia,tegra114-spi.txt 
> >> @@ -6,8 +6,10 @@ Required properties: - interrupts: Should
> >> contain SPI interrupts. - nvidia,dma-request-selector : The Tegra
> >> DMA controller's phandle and request selector for this SPI
> >> controller. -- This is also require clock named "spi" as per
> >> binding document -
> >> Documentation/devicetree/bindings/clock/clock-bindings.txt +-
> >> 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: +  - spi
> > 
> > This is inconsistent with other bindings that require only a
> > single clock entry. I suppose that this is required because of the
> > driver requesting a specifically named clock, in which case that's
> > fine.
> 
> This driver does clk_get(dev, "spi") rather than clk_get(dev, NULL),
> so this requires a specific name. Again, I did consider updating all
> drivers to use names, but decided I didn't want to do even more driver
> changes, but instead just document what was currently required.

Yes, I realized that as well. Oh well, I guess that's part of the "pain"
for not doing it right from the start. Although, admittedly, this really
isn't a big issue.

Thierry