On 9/17/20 11:06 PM, Vaittinen, Matti wrote: > Thanks (again) for review Guenter! > > On Thu, 2020-09-17 at 22:45 -0700, Guenter Roeck wrote: >> On 9/17/20 1:03 AM, Matti Vaittinen wrote: >>> Add Watchdog support for ROHM BD9576MUF and BD9573MUF PMICs which >>> are >>> mainly used to power the R-Car series processors. The watchdog is >>> pinged using a GPIO and enabled using another GPIO. Additionally >>> watchdog time-out can be configured to HW prior starting the >>> watchdog. >>> Watchdog timeout can be configured to detect only delayed ping or >>> in >>> a window mode where also too fast pings are detected. >>> >>> Signed-off-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx> >>> --- >>> drivers/watchdog/Kconfig | 13 ++ >>> drivers/watchdog/Makefile | 1 + >>> drivers/watchdog/bd9576_wdt.c | 295 >>> ++++++++++++++++++++++++++++++++++ >>> 3 files changed, 309 insertions(+) >>> create mode 100644 drivers/watchdog/bd9576_wdt.c > >> >>> + >>> + ret = of_property_read_u32(np, >>> + "hw_margin_ms", &hw_margin); >> >> Line splits are arbitrary. Why is this "hw_margin_ms" and not >> "rohm,hw_margin_ms" ? > > "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 > >> >>> + if (ret) { >>> + if (ret != -EINVAL) >>> + return ret; >>> + >>> + hw_margin = BD957X_WDT_DEFAULT_MARGIN; >>> + } >>> + >>> + ret = of_property_read_u32(np, "rohm,hw-margin-min-ms", >>> &hw_margin_min); >>> + if (ret == -EINVAL) >>> + hw_margin_min = 0; >>> + else if (ret) >>> + return ret; >> >> Please use a single mechanism to handle -EINVAL after >> of_property_read_u32(). > > Sorry Guenter - I am probably a bit slow today but I am unsure if I > understand the suggestion. Do you mean something like: > if (ret) { >>> + if (ret != -EINVAL) >>> + return ret; >>> + >>> + hw_margin = BD957X_WDT_DEFAULT_MARGIN; >>> + } vs. >>> + if (ret == -EINVAL) >>> + hw_margin_min = 0; >>> + else if (ret) >>> + return ret; is not very consistent to me. Guenter > hw_margin_min = 0; > > ret = of_property_read_u32(np, "rohm,hw-margin-min-ms", > &hw_margin_min); > if (ret && ret != -EINVAL) > return ret; > > Other than that - all findings are clear to me. I'll craft a new > version but I'll wait for a while to see if Lee has time to send some > feedback on MFD so I could get both WDG and MFD fixes to same version > :) > > Best Regards > --Matti > > > -- > 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) >
