Re: [PATCH 01/31] ARM: tegra: add missing clock documentation to DT bindings

On Sun, Dec 01, 2013 at 12:05:44PM -0700, Stephen Warren wrote:

On 11/29/2013 04:49 AM, Thierry Reding wrote:

quoted

On Fri, Nov 15, 2013 at 01:53:56PM -0700, Stephen Warren wrote: 
[...]

quoted

@@ -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.

quoted

quoted

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