Re: [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver

[Lee Jones]

On Thu, 09 Oct 2014, Mark Rutland wrote:

> On Wed, Oct 08, 2014 at 10:33:29AM +0100, Lee Jones wrote:
> > Cc: devicetree@xxxxxxxxxxxxxxx
> > Signed-off-by: David Paris <david.paris@xxxxxx>
> > Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/watchdog/st-lpc-wdt.txt    | 30 ++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt
> > new file mode 100644
> > index 0000000..520ce90
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt
> > @@ -0,0 +1,30 @@
> > +STMicroelectronics LPC Watchdog
> > +===============================
> > +
> > +Required properties
> > +
> > +- compatible 	: Must be one of:
> > +			"st,stih407-watchdog"
> > +			"st,stih416-watchdog"
> > +			"st,stih415-watchdog"
> > +			"st,stid127-watchdog"
> > +- reg		: LPC registers base address + range
> 
> s/range/size/
> 
> Please append something like "one entryfor each entry in reg-names".
> 
> > +- reg-names	: Register map "base" and "syscfg-en" are compulsory. "type" is
> > +		  platform dependent and not required for the STiH407
> 
> I don't understand the mention of "type".

This is a typo.  It should be "syscfg-type".

> Additionally, "syscfg-en" looks to be a portion of another device
> (shared system controller?), and probably should be described by
> reference.

Answer below.

> > +- clock-names 	: Should be "lpc_wdt"
> > +- clocks	: Clock used by LPC device
> > +- timeout-sec	: Watchdog timeout in seconds
> > +- st,syscfg	: Syscfg node used to configure CPU reset type and mask
> 
> Does this relate to the syscfg-en entry in the reg proeprty?
 
Yes.  We map these registers with regmap and use the read-in register
values of syscfg-en and syscfg-type to manipulate them.  It's either
that or supplying a property for each read-in, which I like less.

> > +- st,warm_reset	: If present, reset type will be 'warm'. If not, it will be cold
> 
> s/_/-/ in property names.

You're right.  This slipped my gaze.

> Why does this need to be in the binding? It seems like a choice rather
> than a property of the system.

What do you mean?  It's something which needs to be configured at
initialisation time.  If we provide it here, it means that we don't
have to re-complie to change the config.

> > +Example:
> > +	watchdog@fde05000 {
> > +		compatible	= "st,stih416-lpc-watchdog";
> > +		reg		= <0xfde05000 0x1000> <0x204 0x4>;
> > +		reg-names	= "base", "syscfg-en"
> > +		clock-names	= "lpc_wdt";
> > +		clocks 		= <&clk_s_d3_flexgen CLK_LPC_0>;
> > +		timeout-sec	= <600>;
> > +		st,syscfg	= <&syscfg_core>;
> > +		st,warm_reset;
> > +	};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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/