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

[Mike Looijmans <mike.looijmans-Oq418RWZeHk@public.gmane.org>]

On 29-07-15 09:35, Uwe Kleine-König wrote:
> 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.

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.

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.

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

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.

>>> ...
>



Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijmans-yhtFebqMsb9it5bFGTN0CAC/G2K4zDHf@public.gmane.org
Website: www.topicproducts.com

Please consider the environment before printing this e-mail





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