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
pgpvVsTAn7ZW2.pgp
Description: PGP signature
