On Wed, Apr 03, 2024 at 09:34:35AM +0300, Matti Vaittinen wrote: > Hi Guenter, > > First of all, thanks for the review. It was quick! Especially when we speak > of a RFC series. Very much appreciated. > > On 4/2/24 20:11, Guenter Roeck wrote: > > On Tue, Apr 02, 2024 at 04:11:41PM +0300, Matti Vaittinen wrote >> +static int init_wdg_hw(struct wdtbd96801 *w) > > > +{ > > > + u32 hw_margin[2]; > > > + int count, ret; > > > + u32 hw_margin_max = BD96801_WDT_DEFAULT_MARGIN, hw_margin_min = 0; > > > + > > > + count = device_property_count_u32(w->dev->parent, "rohm,hw-timeout-ms"); > > > + if (count < 0 && count != -EINVAL) > > > + return count; > > > + > > > + if (count > 0) { > > > + if (count > ARRAY_SIZE(hw_margin)) > > > + return -EINVAL; > > > + > > > + ret = device_property_read_u32_array(w->dev->parent, > > > + "rohm,hw-timeout-ms", > > > + &hw_margin[0], count); > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (count == 1) > > > + hw_margin_max = hw_margin[0]; > > > + > > > + if (count == 2) { > > > + hw_margin_max = hw_margin[1]; > > > + hw_margin_min = hw_margin[0]; > > > + } > > > + } > > > + > > > + ret = bd96801_set_wdt_mode(w, hw_margin_max, hw_margin_min); > > > + if (ret) > > > + return ret; > > > + > > > + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action", > > > + "prstb"); > > > + if (ret >= 0) { > > > + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, > > > + BD96801_WD_ASSERT_MASK, > > > + BD96801_WD_ASSERT_RST); > > > + return ret; > > > + } > > > + > > > + ret = device_property_match_string(w->dev->parent, "rohm,wdg-action", > > > + "intb-only"); > > > + if (ret >= 0) { > > > + ret = regmap_update_bits(w->regmap, BD96801_REG_WD_CONF, > > > + BD96801_WD_ASSERT_MASK, > > > + BD96801_WD_ASSERT_IRQ); > > > + return ret; > > > + } > > > > I don't see the devicetree bindings documented in the series. > > Seems like I have missed this WDG binding. But after reading your comment > below, I am wondering if I should just drop the binding and default to > "prstb" (shutdown should the feeding be skipped) - and leave the "intb-only" > case for one who actually needs such. > > > I am also a bit surprised that the interrupt isn't handled in the driver. > > Please explain. > > Basically, I just had no idea what the IRQ should do in the generic case. If > we get an interrupt, it means the WDG feeding has failed. My thinking is > that, what should happen is forced reset. I don't see how that can be done > in reliably manner from an IRQ handler. > > When the "prstb WDG action" is set (please, see the above DT binding > handling), the PMIC shall shut down power outputs. This should get the > watchdog's job done. > > With the "intb-only"-option, PMIC will not turn off the power. I'd expect > there to be some external HW connection which handles the reset by HW. > > After all this being said, I wonder if I should just unconditionally > configure the PMIC to always turn off the power (prstb option) should the > feeding fail? Or do someone have some suggestion what the IRQ handler should > do (except maybe print an error msg)? > Other watchdog drivers call emergency_restart() if the watchdog times out and triggers an interrupt. Are you saying this won't work for this system ? If so, please explain. Thanks, Guenter