Re: [PATCH v3 2/2] devicetree: Add Cadence WDT devicetree bindings documentation

[Mark Rutland]

On Tue, Jul 15, 2014 at 07:39:40AM +0100, Harini Katakam wrote:
> Hi Mark,
> 
> On Mon, Jul 14, 2014 at 8:37 PM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> > On Mon, Jul 14, 2014 at 01:16:09PM +0100, Harini Katakam wrote:
> >> Add cadence-wdt bindings documentation.
> >>
> >> Signed-off-by: Harini Katakam <harinik@xxxxxxxxxx>
> >> ---
> >>
> >> v3 changes:
> >> - Change reset property type and improve description.
> >> - Improve description of clocks and interrupts.
> >> - Use watchdog@ in the example.
> >> - Use only cdns compatible string for now.
> >>
> >> v2:
> >> No changes
> >>
> >> ---
> >>  .../devicetree/bindings/watchdog/cadence-wdt.txt   |   27 ++++++++++++++++++++
> >>  1 file changed, 27 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/watchdog/cadence-wdt.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/watchdog/cadence-wdt.txt b/Documentation/devicetree/bindings/watchdog/cadence-wdt.txt
> >> new file mode 100644
> >> index 0000000..ab23e38
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/watchdog/cadence-wdt.txt
> >> @@ -0,0 +1,27 @@
> >> +Zynq Watchdog Device Tree Bindings
> >> +-------------------------------------------
> >> +
> >> +Required properties:
> >> +- compatible         : Should be "cdns,wdt-r1p2".
> >> +- clocks             : Input clock specifier. This should be the ref clk.
> >
> > This wording makes it sound like the watchdog block has more than one
> > clock input. Does it?
> 
> It doesn't. It has one APB clock - named pclk.

Ok. Could we refer to that by name in the description then? The phrase
"This should be the ref clk" makes it sound like there are other clocks
we aren't describing.

> >
> >> +- reg                        : Physical base address and size of WDT registers map.
> >> +- interrupts         : Property with a value describing the interrupt
> >> +                       number. This interrupt is used for indication
> >> +                       when the watchdog times out.
> >
> > Just say "the watchdog timeout interrupt", or (better) use the name of
> > the interrupt from the documentation.
> 
> OK. Yeah I'll do that. The interrupt name is wd_irq.

That sounds good to me.
 
> >
> >> +- interrupt-parent   : Must be core interrupt controller.
> >> +
> >> +Optional properties
> >> +- reset                      : If this property exists, then a reset is done
> >> +                       when watchdog times out.
> >
> > That's a bit of an ambiguous name (too easy to misconstrue as a reset
> > device reference). Do any other watchdogs have similar properties?
> 
> I could change it to "reset-on-timeout" if that better.
> From the documentation of other drivers, there seems to be a reset-type
> property in atmel. Dint find any other reset related properties.
> 

Ok. reset-on-timeout sounds like a better property name to avoid
possible confusion.

That said, what happens if we don't specify the device should reset the
system (but have a timeout-sec property)?

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/