RE: [PATCH 2/2] DT: watchdog: Add ImgTec PDC Watchdog Timer binding documentation

Jude Abraham
2014-11-13T12:58:34Z

Hi Andrew,

Many thanks for reviewing. Please see my inline comments below.

On Wed, Nov 12, 2014 at 7:18 AM, <Naidu.Tellapati@imgtec.com> wrote:
> > From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
> >
> > Add the devicetree binding document for ImgTec PDC Watchdog Timer.
> >
> > Signed-off-by: Jude Abraham <Jude.Abraham@imgtec.com>
> > Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com>

> > diff --git a/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt
> > b/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt
> > new file mode 100644
> > index 0000000..2f13896
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/watchdog/imgpdc-wdt.txt
> >@@ -0,0 +1,18 @@
> > +*ImgTec PowerDown Controller (PDC) Watchdog Timer (WDT)
> > +
> > +Required properties:
> > +- compatible : Should be "img,pistachio-pdc-wdt"

> I don't see anything Pistachio-specific about this driver or binding and it looks like IP revision is probable
> via the WD_CORE_REV register, so I think you can drop the "pistachio".

Ok. We will remove pistachio.

> > +- reg : Should contain WDT registers location and length
> > +- clock-names: Should contain "wdt"

> I believe there are two clocks: the 32kHz watchdog timer operating clock and the system interface gate clock. Perhaps call them "wdt"
> and "sys"?

Ok, We will add sys clock .

> > +- clocks: phandles to input clocks
> > +- interrupts : Should contain WDT interrupt

> Not sure the interrupt will really be necessary.

Ok, We will remove the interrupt.

> > +Examples:
> > +
> > +wdt@18102100 {
> > + compatible = "img,pistachio-pdc-wdt";
> > + reg = <0x18102100 0x20>;

> The TRM I have shows the watchdog registers occupying a full 256 bytes.

OK, will correct it.

> + clocks = <&pdc_wdt_clk>;
> + clock-names = "wdt";
> + interrupts = <0 52 IRQ_TYPE_LEVEL_HIGH>; };
> --
> 1.7.0.4
>

Thanks and regards,
Jude.