Re: [RFC] improve binding for linux,wdt-gpio

Hello Mike,

On Thu, Jul 30, 2015 at 08:15:30AM +0200, Mike Looijmans wrote:

quoted

I'd go for these properties then:

toggle-gpio: the gpio used to stroke the watchdog
enable-gpio: optional, the gpio to enable and disable the watchdog
disable-on-tri-state: optional, signals that the watchdog can
	be stopped by setting the trigger-gpio to tri-state.
compatible, hw_algo and hw_margin_ms: as before.
I think there is no need for a property that signals that the watchdog
is unstoppable. For level-gpio-watchdogs you can do it by setting the
trigger gpio to inactive, and you cannot stop level-gpio-watchdogs
unless enable-gpio or disable-on-tri-state is specified.
If we ever feel the need to describe a gpio watchdog with a input that
starts the device but cannot stop it, I'd suggest to use "start-gpio"
for that one.

I don't see any change in the number of properties required to
describe things. So the driver just gets more complicated,
especially if you want it to be somewhat backward compatible.

I think the backward compatibility stuff isn't that hard to implement,
but I'd like to first agree on a sane description.

The way you describe how one could get the "always-running" effect
doesn't really sound intuitive. It's much easier to just have a
property that plainly explains that the watchdog is unstoppable,
than that that is the result of combining a bunch of seemingly
unrelated properties together to get the driver to do what needs to
be done.

It's not more complicated. In the state as currently documented you
have:

	def disable():
		if (has_property(always-running)):
			it's unstoppable
		elif hw_algo == "toggle":
			set to tri-state
		elif hw_algo == "level":
			set to inactive

With my suggested binding it's:

	def disable():
		if (has_property(disable-on-tri-state)):
			set to tri-state
		elif (has_property(enable-gpio)):
			set enable-gpio to inactive
		else:
			it's unstoppable

This is quite similar, but it handles more hardware (i.e. the chips with
an enable gpio). The only added difficulty I see is that you might have
to handle toggle-gpio (or input-gpio in the latest suggestion) ==
enable-gpio to model chips that are disabled by a certain level of the
input gpio (as currently modelled by hw_algo = level without
always-running).

(So yes, it's easier with the current model to find out if the watchdog
is unstoppable but that alone doesn't help you.)

quoted

I think we agree here, that "always-running" is a hardware property.

I'd that "always-running" describes both. The driver must be always
stroking (what a nice word) the watchdog because the watchdog is
always watching.

The driver-part is about policy. Maybe you'd prefer the driver to not
autostroke because it should reset the machine instead.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html