Re: [PATCH v1 1/6] dt_bindings: mfd: Add ROHM BD9576MUF and BD9573MUF PMICs

Hi dee Ho peeps!

On Thu, 2020-09-24 at 09:12 +0300, Matti Vaittinen wrote:

On Wed, 2020-09-23 at 08:27 -0600, Rob Herring wrote:

quoted

On Sat, Sep 19, 2020 at 5:46 AM Vaittinen, Matti
[off-list ref] wrote:

quoted

Thanks Rob for taking a look at this!

On Fri, 2020-09-18 at 11:28 -0600, Rob Herring wrote:

quoted

On Thu, Sep 17, 2020 at 11:01:52AM +0300, Matti Vaittinen
wrote:

quoted

Add bindings for ROHM BD9576MUF and BD9573MUF PMICs. These
PMICs are primarily intended to be used to power the R-Car
series
processors. They provide 6 power outputs, safety features and
a
watchdog with two functional modes.

Signed-off-by: Matti Vaittinen <
matti.vaittinen@fi.rohmeurope.com>
---
 .../bindings/mfd/rohm,bd9576-pmic.yaml        | 129
++++++++++++++++++
 1 file changed, 129 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/mfd/rohm,bd9576-pmic.yaml

// Snip

quoted

quoted

quoted

quoted

+
+  hw_margin_ms:

Needs a vendor prefix.

s/_/-/

quoted

+    minimum: 4
+    maximum: 4416
+    description: Watchog timeout in milliseconds

Maybe the words in the description should be in the property
name
as
I don't see how 'h/w margin' relates to 'watchdog timeout'.

The hw_margin_ms is an existing property. As I wrote to Guenter:
"hw_margin_ms" is an existing binding for specifying the maximum
TMO in
HW (if I understood it correctly). (It is used at least by the
generig
GPIO watchdog) I thought it's better to not invent a new vendor
specific binding when we have a generic one.

https://elixir.bootlin.com/linux/v5.9-rc2/source/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt

That one is odd and I haven't found an actual user of it. It would
make more sense as a collection of properties devices could use
rather
than a virtual device.

I think I'd do something like 'watchdog-ping-time-msec' that can be
either '<min> <max>' or '<max>'.

Your suggestion looks good to me. If we introduce such then it would
make sense to add handling for this in GPIO watchdog too.

What I do wonder is how "hw_margin_ms" is unused? I see it is a
required property for GPIO watchdog. The GPIO WDG probe seems to
actually error out if reading this property fails with any error. I
would assume the GPIO WDG is used somewhere? Hence I am a bit afraid
of
touching it. Breaking existing setups would not be nice.

Guenter - how do you see this? Should we leave GPIO WDG as it is,
convert it to use this new binding Rob suggested - or support both
the
old and new (at least for some time) in the driver - and possibly
print
a warning when old is used?

And one thing more - I don't think the 'watchdog-ping-time-msec' is
best candidate as it sounds like the time when one should ping the WDG
(SW feature). We already have the timeout-sec defined for that. This
new property is to configure/advertice the HW time limit - Eg, time
when WDG takes action if it has not been pinged (for max) or takes
action if WDG is pinged too quickly (min). Thus I liked hw-margin
better than ping-time. (For example with hw-margin <500ms> ... <4000ms>
the ping-time 1000 ms would be just fine.

Couple of things I would like to get opinions for ... 

1. Corect location for this binding - and should it be vendor specific
or generic?
 - I wonder if I should put this new property in rohm-bd9576.yaml? 
 - Should it be vendor specific? 
 - Or should I put it in watchdog.yaml and make it generic?

I think it should be generic as many wdg chips implement the timeout
configuration.

2. Should we extend WDG core to parse this property if it is placed in
watchdog.yaml?
2a) And should we extend watchdog core to call the driver callback for
setting timeout if it finds the <max> tmo?
2b) Should we extend driver IF to allow callback for setting min tmo?
2c) Current tmo setting callback uses units of seconds. Should we
support setting TMO in ms? I think it might make sense for few specific
Linux setups. (I know people use Linux for things that are almost RT -
no matter how clever that is. So some might benefit from sub-second
scale wdg window).

Thoughts?

Best Regards
	Matti Vaittinen


--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)