Hello Guenter,
On Tue, Jul 28, 2015 at 02:21:55PM -0700, Guenter Roeck wrote:
> 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.
For hw_algo = "level" the inactive level of the gpio disables the
watchdog (and resets the counter). So always-running doesn't make sense
for this type.
> Of course, 'always-running' _may_ describe driver behavior, but that doesn't
Well in the current state of the binding document we have:
add this flag to have the driver keep toggling the signal
without a client.
Sure it is meant to describe a hardware specific property, but it talks
about the driver.
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.
> 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.
Yeah, I'm aware of that. For this driver however I wouldn't expect that
you have a dedicated enable-gpio if you cannot disable the device with it.
For hw_algo = "level" there is probably no device with an enable input
and for hw_algo = "toggle" any sane hardware engineer would simply
enable the watchdog once the first toggle is detected on WDI. (OK,
assuming hardware engineers being sane turned out to be a weak argument
often in the past.)
> 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.
I think we agree here, that "always-running" is a hardware property.
> > 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.
Yeah, I'm aware of that. I talked about that in #armlinux yesterday, and
Mark Brown (added to Cc:) said:
Well, I'd prefer to change the standard TBH and allow singular.
This keeps coming up and causing confusion for no good reason.
Sounds sensible in my ears.
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
