[PATCH v3 2/6] dt-bindings: add mtk-timer bindings

[Sören Brinkmann]

On Tue, 2014-05-13 at 04:08PM +0200, Matthias Brugger wrote:
> 2014-05-13 15:08 GMT+02:00 Sören Brinkmann <soren.brinkmann at xilinx.com>:
> > Hi Matthias,
> >
> > On Tue, 2014-05-13 at 01:49AM +0200, Matthias Brugger wrote:
> >> Add binding documentation for the General Porpose Timer driver of
> >> the Mediatek SoCs.
> >>
> >> Signed-off-by: Matthias Brugger <matthias.bgg at gmail.com>
> >> ---
> >>  .../devicetree/bindings/timer/mediatek,mtk-timer.txt   | 18 ++++++++++++++++++
> >>  1 file changed, 18 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/timer/mediatek,mtk-timer.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/timer/mediatek,mtk-timer.txt b/Documentation/devicetree/bindings/timer/mediatek,mtk-timer.txt
> >> new file mode 100644
> >> index 0000000..d0f2df3
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/timer/mediatek,mtk-timer.txt
> >> @@ -0,0 +1,18 @@
> >> +Mediatek MT6589, MT6577 and MT6572 Timers
> >> +---------------------------------------
> >> +
> >> +Required properties:
> >> +- compatible: Should be "mediatek,mtk6589-timer"
> >> +- reg: Should contain location and length for timers register.
> >> +- clocks: phandle to the clock source; the first refers to a 13 MHz fixed
> >> +             system clock and the second handle to a 32 KHz fixed RTC
> >> +             clock.
> >
> > Are these frequencies mandatory to the timer or an implementation
> > detail of the SOC you're working with? I suspect, it might be possible
> > to see the same timer in a different SOC implementation with different
> > frequencies? In that case - or probably in general - the frequencies
> > should not be part of the binding, IMHO.
> 
> I had a look at the implementation of several Mediatek SoCs and all of
> them have the timer with the same clock connections.
> I don't know if the IP can be or is actually used with other clocksources.
> I can try to reformulate the description to make the clock constraints
> sound less mandatory.
> Although I would prefer to leave a mention about the clock frequencies
> (like e.g. in the allwinner,sun4i-timer).

Since the bindings should be generic, I think the frequencies shouldn't
be here. From DT/HW perspective their frequencies is irrelevant and a
pure implementation detail, IMHO.

> 
> >
> >> +
> >> +Examples:
> >> +
> >> +     timer {
> >> +             compatible = "mediatek,mtk6589-timer";
> >> +             reg = <0x10008000 0x80>;
> >> +             interrupts = <GIC_SPI 113 IRQ_TYPE_LEVEL_LOW>;
> >> +             clocks = <&system_clk>, <&rtc_clk>;
> >
> > Might be just my personal preference, but you could also add the clock-names
> > property which would relax the ordering requirement a bit and would
> > clearly  identify the IP's clocks.
> 
> In the v1 of the patch series I had the clock names included [1] but
> because of comments of others I removed them [1].
> Actually the driver just use the first clock, but from my
> understanding the DT reflects as well the HW configuration and for
> this reason I put the two clocks in the DT.
> 
> [1] http://thread.gmane.org/gmane.linux.drivers.devicetree/69155/

I had cases where the same IP was implemented by different SOC vendors
differently. In that cases having the clock-names in the bindings is
extremely helpful when you try to match the binding with the IP docs.
Hence, I'd recommend being explicit and list the names.

	Sören