Re: [RESEND PATCHv2] watchdog: dw: Enable OF support for DW watchdog timer.

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Oct 22, 2013 at 11:57:34AM +0100, Dinh Nguyen wrote:
> Hi Mark,
> 
> On 10/22/13 5:39 AM, Mark Rutland wrote:
> > On Tue, Oct 22, 2013 at 01:41:20AM +0100, Guenter Roeck wrote:
> >> On 10/21/2013 04:05 AM, Mark Rutland wrote:
> >>> On Wed, Oct 02, 2013 at 07:44:32PM +0100, dinguyen@altera.com wrote:
> >>>> From: Dinh Nguyen <dinguyen@altera.com>
> >>>>
> >>>> Add device tree support to the DW watchdog timer.
> >>>>
> >>>> Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
> >>>> Acked-by: Jamie Iles <jamie@jamieiles.com>
> >>>> Reviewed-by: Pavel Machek <pavel@denx.de>
> >>>> Cc: Guenter Roeck <linux@roeck-us.net>
> >>>> Cc: Jamie Iles <jamie@jamieiles.com>
> >>>> Cc: Viresh Kumar <viresh.linux@gmail.com>
> >>>> Cc: Wim Van Sebroeck <wim@iguana.be>
> >>>> Cc: Pavel Machek <pavel@denx.de>
> >>>> Cc: Rob Herring <rob.herring@calxeda.com>
> >>>> Cc: Pawel Moll <pawel.moll@arm.com>
> >>>> Cc: Mark Rutland <mark.rutland@arm.com>
> >>>> Cc: Stephen Warren <swarren@wwwdotorg.org>
> >>>> Cc: Ian Campbell <ian.campbell@citrix.com>
> >>>> Cc: devicetree@vger.kernel.org
> >>>> Cc: linux-watchdog@vger.kernel.org
> >>>> ---
> >>>> v2:
> >>>> - Use of_match_ptr() for of_match_table
> >>>> ---
> >>>>    .../devicetree/bindings/watchdog/dw_wdt.txt        |   16 ++++++++++++++++
> >>>>    drivers/watchdog/dw_wdt.c                          |    8 ++++++++
> >>>>    2 files changed, 24 insertions(+)
> >>>>    create mode 100644 Documentation/devicetree/bindings/watchdog/dw_wdt.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/watchdog/dw_wdt.txt b/Documentation/devicetree/bindings/watchdog/dw_wdt.txt
> >>>> new file mode 100644
> >>>> index 0000000..29e150b
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/watchdog/dw_wdt.txt
> >>>> @@ -0,0 +1,16 @@
> >>>> +Synopsys Designware Watchdog Timer
> >>>> +
> >>>> +Required Properties:
> >>>> +
> >>>> +- Compatiblity	: "snps,dw-wdt"
> >>> This should presumably be:
> >>>
> >>> - compatbile: should contain "snps,dw-wdt"
> >>>
> >> Hi Mark,
> >>
> >> s/compatbile/compatible/ :-)
> > Indeed :)
> Changed for v3.
> >
> >> "must be" or "should contain" ? I see both in various bindings.
> >> Is there a preference ?
> > I would prefer "should contain" -- it doesn't preclude future compatible
> > variants.
> Oops..v3 has "should be"...will change to "should contain" in v4.

Cheers.

> >
> >>>> +- reg		: Base address of the watchdog timer register.
> >>> And the size...
> >>>
> >>>> +
> >>>> +Example:
> >>>> +
> >>>> +	watchdog0: wd@ffd02000 {
> >>>> +		compatible = "snps,dw-wdt";
> >>>> +		reg = <0xffd02000 0x1000>;
> >>>> +		interrupts = <0 171 4>;
> >>> This wasn't mentioned.
> >>>
> >>> Is it necessary?
> >>>
> >>   From looking into the code ...
> >>
> >> The driver doesn't use interrupts, so I guess the answer is no. Cut-and-paste error, maybe ?
> > Does the device have any interrupts, even if they're unused?
> Yes, the device does have an interrupt. Should I add it as an optional 
> property?

Yes please, given it's easy to add. We might want to use it in future so it
would be nice to have now.

> >
> >>> Is it the only interrupt?
> >>>
> >>>> +		clocks = <&per_base_clk>;
> >>> Similarly, is this the only clock?
> >>>
> >> The driver uses one clock, and it is mandatory.
> > Is this the only clock into the unit?
> >
> > Does it have a name?
> Yes, this is is the only clock into the unit. No, it does not have name.

Ok.

Thanks,
Mark.