Hi Uwe,
On Tue, Jul 28, 2015 at 10:33:48PM +0200, Uwe Kleine-König wrote:
> This is just a suggestion up to now, I don't have any code yet to share.
>
> Apart from minor rewording to make the document easier to understand and
> less ambiguous the relevant changes are:
>
> - add an "enable-gpio" property.
> I admit the device I'm currently working with doesn't have this.
> Still I imagine this to be a common hardware property. I added it
> mainly to demonstrate the shortcomings of the existing binding.
> - rename "gpios" to "trigger-gpio"
> This is more descriptive. And given there is "enable-gpio" now, too,
> using plain "gpios" seems wrong.
> - deprecate always-running
> Apart from the description describing the driver behaviour and not
> the device property, always-running only seems to make sense in
> combination with hw_algo = "level" and in reality should only
> invalidate the sentence: "The watchdog timer is disabled when GPIO is
> left floating or connected to a three-state buffer."
always-running is meant to indicate that the watchdog can not be stopped
(meaning a timer has to be used to send keepalives while the watchdog
device is closed). The documentation specifically states that.
"If the watchdog timer cannot be disabled ..."
How would you express that condition without always-running or a similar
attribute ? I am also not sure how that relates to hw_algo; I thought
those properties are orthogonal.
Of course, 'always-running' _may_ describe driver behavior, but that doesn't
have to be the case. There are lots of watchdogs out there which can not be
stopped. Some of them run all the time, others can not be stopped once started.
That gets us into the rat-hole of arguing if property X describes driver
behavior or the hardware, and of rejecting properties because they may
be driver descriptions in some use cases (because 'always-running' can
be set even if the hardware doesn't mandate it, making it driver behavior).
I'd rather not go there.
> With this semantic using a property "disable-on-tri-state" sounds
> more sensible. And note that even the following would make sense
> hardware-wise, while the device tree looks stupid:
>
> watchdog {
> compatible = "linux,wdt-gpio";
> trigger-gpio = ...;
> hw_algo = "toggle";
> always-running;
> enable-gpio = ...;
> };
>
> (i.e. always-running, but disable possible by a dedicated gpio.)
>
It might also mean that _enable_ is possible with a dedicated gpio.
That doesn't mean it can be stopped once started. Again, there are lots
of watchdogs out there which can be enabled/started but not stopped.
> I'm aware that using ...-gpios is more common than ...-gpio. I don't
> feel strong here, but as only a single gpio makes sense here, having
> -gpios seems wrong.
>
Documentation/devicetree/bindings/gpio/gpio.txt states that gpio pin
references should be named <name>-gpios. It even lists examples such as
enable-gpios = <&gpio2 2>;
I thought this was a hard rule, and I seem to recall requests to change
something-gpio to something-spios, but I may be wrong.
Thanks,
Guenter
> Also I considered renaming hw_margin_ms and hw_algo to use - instead of
> _. Doing this might even ease to implement the changes above in a
> compatible way. I.e. assume the watchdog can be disabled by tristating
> the gpio if the description uses underscores instead of hyphen.
>
> Feedback very welcome!
>
> Best regards
> Uwe
>
> ---
> .../devicetree/bindings/watchdog/gpio-wdt.txt | 37 ++++++++++++++--------
> 1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> index 198794963786..ceb1a5f95f44 100644
> --- a/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt
> @@ -2,27 +2,36 @@
>
> Required Properties:
> - compatible: Should contain "linux,wdt-gpio".
> -- gpios: From common gpio binding; gpio connection to WDT reset pin.
> -- hw_algo: The algorithm used by the driver. Should be one of the
> +- trigger-gpio: reference to the gpio connected to watchdog's input pin
> + (typically called WDI).
> +- hw_algo: The algorithm used by the device. Should be one of the
> following values:
> - - toggle: Either a high-to-low or a low-to-high transition clears
> - the WDT counter. The watchdog timer is disabled when GPIO is
> - left floating or connected to a three-state buffer.
> - - level: Low or high level starts counting WDT timeout,
> - the opposite level disables the WDT. Active level is determined
> - by the GPIO flags.
> -- hw_margin_ms: Maximum time to reset watchdog circuit (milliseconds).
> + - toggle: Both a high-to-low and a low-to-high transition clear
> + the watchdog counter.
> + - level: Low or high level starts counting watchdog timeout,
> + the opposite level disables the watchdog. Active level is determined
> + by the GPIO flags of the trigger-gpio (with active meaning the watchdog is
> + enabled).
> +- hw_margin_ms: Maximum time to reset watchdog circuit in milliseconds.
>
> Optional Properties:
> -- always-running: If the watchdog timer cannot be disabled, add this flag to
> - have the driver keep toggling the signal without a client. It will only cease
> - to toggle the signal when the device is open and the timeout elapsed.
> +- enable-gpio: Reference to a gpio that when inactive stops the watchdog.
> +- disable-on-tri-state: property that signals that the watchdog can be stopped
> + by setting the trigger-gpio to tri-state.
> +
> +Deprecated Properties:
> +- always-running: This property signals the watchdog timer cannot be disabled.
> + Without this property the watchdog is expected to turn off for hw_algo=toggle
> + watchdogs when the gpio is set to tri-state.
> + Don't use it for new device descriptions as it is misleading in the presence
> + of an enable-gpio.
> +- gpios: deprecated alias of trigger-gpio
>
> Example:
> watchdog: watchdog {
> /* ADM706 */
> - compatible = "linux,wdt-gpio";
> - gpios = <&gpio3 9 GPIO_ACTIVE_LOW>;
> + compatible = "adi,adm706", "linux,wdt-gpio";
> + trigger-gpio = <&gpio3 9 GPIO_ACTIVE_LOW>;
> hw_algo = "toggle";
> hw_margin_ms = <1600>;
> };
> --
> 2.1.4
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
