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

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Hello Mike,

On Thu, Jul 30, 2015 at 08:15:30AM +0200, Mike Looijmans wrote:
> >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.)

> >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